Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moodle 4.5 incompatibility if redis session handler is configured. #503

Open
benyovszky opened this issue Nov 18, 2024 · 4 comments
Open

Comments

@benyovszky
Copy link
Contributor

  • moosh version: 1.25
  • moodle version: 4.5
    • database: mysql
  • php version: 8.2
  • operating system: debian 12.8

Actual behaviour

I have a moodle instance with confiured redis session cache in config.php:
$CFG->session_handler_class = '\core\session\redis';

Then I execute ANY moosh command, it fails:

root@install:/var/www/mdl# sudo -u www-data /opt/moosh/moosh.php filter-set multilang 1

Warning: session_set_save_handler(): Session save handler cannot be changed when a session is active in /var/www/mdl/lib/classes/session/redis.php on line 235
!!! Setting up of redis session failed. Please notify the server administrator. !!!
!!
Error code: redissessionhandlerproblem !!
!! Stack trace: * line 237 of /lib/classes/session/redis.php: core\session\exception thrown

  • line 750 of /lib/classes/session/redis.php: call to core\session\redis->init()
  • line 688 of /lib/classes/session/redis.php: call to core\session\redis->init_redis_if_required()
  • line 996 of /lib/classes/session/manager.php: call to core\session\redis->destroy()
  • line 636 of /lib/classes/session/manager.php: call to core\session\manager::destroy()
  • line 4078 of /lib/moodlelib.php: call to core\session\manager::login_user()
  • line 303 of /opt/moosh_new/moosh.php: call to complete_user_login()
    !!

Expected behaviour

Run, without error.

Steps to reproduce

  • Install a redis server to localhost.
  • Install a moodle 4.5.
  • Edit config.php, add the following entries
    $CFG->session_handler_class = '\core\session\redis';
    $CFG->session_redis_host = '127.0.0.1';
    $CFG->session_redis_port = 6379;
    $CFG->session_redis_prefix = 'mdl2_session_';
  • Install moosh
  • Execute any moosh command
@benyovszky
Copy link
Contributor Author

Related:

@tmuras
Copy link
Owner

tmuras commented Nov 18, 2024

What if we add a global option to moosh to over-write any $CFG setting in config.php?
For example if you run:
moosh -o "session_handler_class=NULL"
moosh would then unset $CFG->session_handler_class

@benyovszky
Copy link
Contributor Author

benyovszky commented Nov 19, 2024

  • I tried that, as it seemed to be a solution (or a workaround, without really seeing the consequences of mixing different types of sessions considering reports/logs).
    • But it gave me the same error. Not sure why.
  • Another option is to patch Moodle to check if the session is already active:
    • /lib/classes/session/redis.php#235
      if (session_status() !== PHP_SESSION_ACTIVE) {
      $result = session_set_save_handler($this);
      if (!$result) {
      throw new exception('redissessionhandlerproblem', 'error');
      }
      }
    • /lib/classes/session/database.php#64
      if (session_status() !== PHP_SESSION_ACTIVE) {
      $result = session_set_save_handler($this);
      if (!$result) {
      throw new exception('dbsessionhandlerproblem', 'error');
      }
      }
    • This helps to eliminate the error message.
    • Here, I also see some unknown consequences; further testing/analytics are required
  • The source of the issue is that Moosh already sent some output before changing the handler. Identifying those and, if possible, eliminating them could solve the issue. This would be the cleanest solution.

@benyovszky
Copy link
Contributor Author

benyovszky commented Nov 19, 2024

Ok, I have found a solution:
Just add the following to moosh.php at line 299, right before session_start();
\core\session\manager::destroy_all();
There is no further modification needed.

This will force the script to use the configured session handler before starting the session. Otherwise, the built-in file handler will be used.

  • This might break backward compatibility. Older Moodle versions used \core\session\manager::kill_all_sessions(), which has existed since Moodle 2.6. kill_all_sessions() has been deprecated since 4.5, but still works.
  • I need to do further investigation if it has any negative effect.

I'm pretty sure that with better knowledge or further investigation, a cleaner solution can be found to force moodle to use the session handler from moosh when starting the session.

tmuras added a commit that referenced this issue Jan 14, 2025
Option to not login as admin user when executing a command.
Shuold help with #503.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants