Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-69684

It's possible to hold a redis session lock forever

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Reopened
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 3.9.3, 3.10, 3.11
    • Fix Version/s: None
    • Component/s: Libraries
    • Database:
      Any
    • Testing Instructions:
      Hide

      You are required to have a Redis server running and configured to work with your web server in order to perform this test.
      For ubuntu:

      apt-get install php-redis redis-server
      service apache2 restart

      Edit config.php and add the following:

      $CFG->session_handler_class = '\core\session\redis';
      $CFG->session_redis_host = '127.0.0.1';

       

      Launch redis-cli and run the monitor command to watch a session get created.

      Login to moodle, inspect the output of redis-cli. Verify that session locks are created with expiry time set by $CFG->session_redis_lock_expire set in config.php.

      Show
      You are required to have a Redis server running and configured to work with your web server in order to perform this test. For ubuntu: apt-get install php-redis redis-server service apache2 restart Edit config.php and add the following: $CFG->session_handler_class = '\core\session\redis'; $CFG->session_redis_host = '127.0.0.1';   Launch redis-cli and run the monitor command to watch a session get created. Login to moodle, inspect the output of redis-cli. Verify that session locks are created with expiry time set by $CFG->session_redis_lock_expire set in config.php.
    • Affected Branches:
      MOODLE_310_STABLE, MOODLE_311_STABLE, MOODLE_39_STABLE
    • Pull 3.11 Branch:
      MDL-69684_MOODLE311
    • Pull Master Branch:

      Description

      in /lib/classes/session/redis.php
      a session lock is acquired from redis with the following code:

      $haslock = $this->connection->setnx($lockkey, $whoami);
      if ($haslock) {
          $this->locks[$id] = $this->time() + $this->lockexpire;
          $this->connection->expire($lockkey, $this->lockexpire);
          return true;
      }

      Since the lock is acquired and expired in two separate calls it's possible for it to lock out a user indefinitely if moodle is taken down.

      It should use the atomic call to set the expiry at the same time.

      $haslock = $this->connection->set($lockkey, $whoami, ['nx', 'ex'=>$this->lockexpire]);
      if ($haslock) {
          $this->locks[$id] = $this->time() + $this->lockexpire;
          return true;
      }
      

      We've seen this happen once in production. I'm more than happy to make a pr for this issue.

       

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              Unassigned Unassigned
              Reporter:
              jamie@agilicus Jamie
              Peer reviewer:
              Ferran Recio
              Integrator:
              Adrian Greeve
              Participants:
              Component watchers:
              Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Ilya Tregubov, Sara Arjona (@sarjona)
              Votes:
              3 Vote for this issue
              Watchers:
              8 Start watching this issue

                Dates

                Created:
                Updated:

                  Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 55 minutes
                  55m