Moodle
  1. Moodle
  2. MDL-31501

New session infrastructure - file, database and memcached storage

    Details

    • Testing Instructions:
      Hide

      Repeat following for all 3 session handlers (database, file, memcached), see config-dit.php for configuration options.

      1. run phpunit tests
      2. install moodle
      3. upgrade moodle from 2.2 and 2.5
      4. login - logout - login - logout
      5. login in multiple browsers with multiple accounts - verify the sessions table gets filled with all sessions and the timemodified column is updated every 20 seconds
      6. login in one browser and delete or suspend the account as admin from different browser - user should be kicked out
      7. verify session locking works (please note the first http requests is not locked) - the easiest way is to create a script with require('config.php') followed by sleep(30);

      Other tests:

      1. verify that file based handler is used by default in new installs
      2. verify the "database sessions option" disappears if you put $CFG->session_handler_class into your config.php
      3. be creative and try to come up with more session related testing
      Show
      Repeat following for all 3 session handlers (database, file, memcached), see config-dit.php for configuration options. run phpunit tests install moodle upgrade moodle from 2.2 and 2.5 login - logout - login - logout login in multiple browsers with multiple accounts - verify the sessions table gets filled with all sessions and the timemodified column is updated every 20 seconds login in one browser and delete or suspend the account as admin from different browser - user should be kicked out verify session locking works (please note the first http requests is not locked) - the easiest way is to create a script with require('config.php') followed by sleep(30); Other tests: verify that file based handler is used by default in new installs verify the "database sessions option" disappears if you put $CFG->session_handler_class into your config.php be creative and try to come up with more session related testing
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w39_MDL-31501_m26_sessionreboot
    • Story Points:
      100
    • Rank:
      109
    • Sprint:
      BACKEND Sprint 4

      Description

      The OU finds memcache for session handling to be important for performance. Here is our code to be commented on, and hopefully, if good enough, added to core.

      Edit: This ticket has been highjacked for memcached session handler instead of memcache.

        Issue Links

          Activity

          Hide
          Jenny Gray added a comment -

          http://moodle.org/mod/forum/discuss.php?d=183786 suggests that Petr thinks this needs work because there are missing features, but which ones are missing?

          Show
          Jenny Gray added a comment - http://moodle.org/mod/forum/discuss.php?d=183786 suggests that Petr thinks this needs work because there are missing features, but which ones are missing?
          Hide
          Tim Hunt added a comment -

          Here is the OU code, cleaned up a bit, and more integrated into Moodle core. Petr, it would be great if you could take a look and let us know what you think.

          Show
          Tim Hunt added a comment - Here is the OU code, cleaned up a bit, and more integrated into Moodle core. Petr, it would be great if you could take a look and let us know what you think.
          Hide
          Tim Hunt added a comment -

          OK, now I have merged this to master, which makes more sense than having a 2.1 branch. (The OU is currently on 2.1, which is why I did that branch first.)

          I can't actually test this, because I don't have memcache installed. However, I have tested that there are no regressions.

          What needs to happen next.

          1. We need someone who knows about this sort of thing (that means you Petr ) to comment on whether the general idea and implementation of the patch is right.

          2. We need some people to test that this actually works, and then compare performance between this and DB sessions.

          Show
          Tim Hunt added a comment - OK, now I have merged this to master, which makes more sense than having a 2.1 branch. (The OU is currently on 2.1, which is why I did that branch first.) I can't actually test this, because I don't have memcache installed. However, I have tested that there are no regressions. What needs to happen next. 1. We need someone who knows about this sort of thing (that means you Petr ) to comment on whether the general idea and implementation of the patch is right. 2. We need some people to test that this actually works, and then compare performance between this and DB sessions.
          Hide
          Andrew Nicols added a comment - - edited

          Quite excited to see this.

          I like the way that this has been implemented, and it seems to work well.

          It doesn't fail so gracefully if the memcache hosts do not respond. I tried stopping my memcached server and got a blank page with zend_mm_heap corruption errors in my apache log.

          I guess one solution to this would be to start a memcached connection in the can_work() function, but this would be adding an additional connect and thus reducing some of the effectiveness of using memcached in the first place. Additionally, anyone who is using memcached is likely to know what they're doing and hopefully to be monitoring their memcached servers for signs of trouble.

          That said, it might be worth expanding upon the can_work() test when selecting sessions. If a user has the memcached extensions installed on the server they're using, but has not configured memcached in moodle and inadvertently turns it on, the only way to get a working login would be to fettle the database.

          Andrew

          Show
          Andrew Nicols added a comment - - edited Quite excited to see this. I like the way that this has been implemented, and it seems to work well. It doesn't fail so gracefully if the memcache hosts do not respond. I tried stopping my memcached server and got a blank page with zend_mm_heap corruption errors in my apache log. I guess one solution to this would be to start a memcached connection in the can_work() function, but this would be adding an additional connect and thus reducing some of the effectiveness of using memcached in the first place. Additionally, anyone who is using memcached is likely to know what they're doing and hopefully to be monitoring their memcached servers for signs of trouble. That said, it might be worth expanding upon the can_work() test when selecting sessions. If a user has the memcached extensions installed on the server they're using, but has not configured memcached in moodle and inadvertently turns it on, the only way to get a working login would be to fettle the database. Andrew
          Hide
          Andrew Nicols added a comment -

          At the very least, the can_work() function should check that the result of get_memcache_hosts() is not empty

          Show
          Andrew Nicols added a comment - At the very least, the can_work() function should check that the result of get_memcache_hosts() is not empty
          Hide
          Andrew Nicols added a comment -

          The help text for the memcachesessionhosts setting should probably include details of how to add multiple hosts (e.g. comma-separated)

          Show
          Andrew Nicols added a comment - The help text for the memcachesessionhosts setting should probably include details of how to add multiple hosts (e.g. comma-separated)
          Hide
          Andrew Nicols added a comment -

          Ahem - ignore my comment re checking for memcachesessionhosts – I'm clearly not quite reading things properly this afternoon!

          Show
          Andrew Nicols added a comment - Ahem - ignore my comment re checking for memcachesessionhosts – I'm clearly not quite reading things properly this afternoon!
          Hide
          Tim Hunt added a comment -

          Andrew, I am unlikely to have time to do any more work on this any time soon. If you feel you can improve anything about this patch, I am very happy to have a look at any code you want to propose.

          Show
          Tim Hunt added a comment - Andrew, I am unlikely to have time to do any more work on this any time soon. If you feel you can improve anything about this patch, I am very happy to have a look at any code you want to propose.
          Hide
          Dan Poltawski added a comment -

          OK, this works, the OU are running it in production and its useful for big sites. Stealing the peer review off Petr to look at it.

          Show
          Dan Poltawski added a comment - OK, this works, the OU are running it in production and its useful for big sites. Stealing the peer review off Petr to look at it.
          Hide
          Dan Poltawski added a comment -

          I've rebased this and put in some fixes I found (mentioned below):
          https://github.com/danpoltawski/moodle/compare/MDL-31501

          Summary is it seems to be working well for me and I think we should get it in.

          Max Session Size
          I thought that a barrier was going to be the maximum object size which can be stored by memcached (1MB by default). Its possible to get a bigger session than that (the comment about max_packet_size in myslql).

          For an example of this go to: Admin > XMLDB > load lib/db/ and try an edit a file and you get a session of about 1.5MB. memcached doesn't like it:

          30: Client using the ascii protocol
          <30 add memc.sess.key.lock.i8jbm5mqfdr5fovdcmf00scrm2 0 1336791610 1
          >30 STORED
          <30 get memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2 
          >30 sending key memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2
          >30 END
          <30 set memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2 0 1440 1433468
          >30 SERVER_ERROR object too large for cache
          <30 delete memc.sess.key.lock.i8jbm5mqfdr5fovdcmf00scrm2
          >30 NOT_FOUND
          <30 quit
          <30 connection closed.
          
          Warning: Unknown: Failed to write session data (memcached). Please verify that the current setting of session.save_path is correct (memcached.lcoal:11211) in Unknown on line 0
          

          Of course I think we really need to get our session size down below that as much as we can, but we are not there yet, and who knows where else our session might be bloated.

          Googling around pointed to that being a hard limit in memcached, pointing to an old FAQ entry (e.g. http://stackoverflow.com/questions/5349/memcached-chunk-limit) suggesting its a relatively hard limit. However, it looks like there is now an option for this! So I tried memcached server with -I 4194304 and indeed it fixes it. So there is dfinitely a way to get memached to work for our mamoth sessions.

          Some other comments:

          1. Note that from that transaction above you can see that the php driver handles the locking of the session
          2. Save handler is memcached not memcache
          3. save_path doesn't have tcp://
          4. We probably need to think about the help texts. That sessionhandler text is quite long, maybe it be inappropiate to put some introductory text in the session handlers page?
          5. Similarly the setting this text seems redundant for each entry: 'for session information'
          Show
          Dan Poltawski added a comment - I've rebased this and put in some fixes I found (mentioned below): https://github.com/danpoltawski/moodle/compare/MDL-31501 Summary is it seems to be working well for me and I think we should get it in. Max Session Size I thought that a barrier was going to be the maximum object size which can be stored by memcached (1MB by default). Its possible to get a bigger session than that (the comment about max_packet_size in myslql). For an example of this go to: Admin > XMLDB > load lib/db/ and try an edit a file and you get a session of about 1.5MB. memcached doesn't like it: 30: Client using the ascii protocol <30 add memc.sess.key.lock.i8jbm5mqfdr5fovdcmf00scrm2 0 1336791610 1 >30 STORED <30 get memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2 >30 sending key memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2 >30 END <30 set memc.sess.key.i8jbm5mqfdr5fovdcmf00scrm2 0 1440 1433468 >30 SERVER_ERROR object too large for cache <30 delete memc.sess.key.lock.i8jbm5mqfdr5fovdcmf00scrm2 >30 NOT_FOUND <30 quit <30 connection closed. Warning: Unknown: Failed to write session data (memcached). Please verify that the current setting of session.save_path is correct (memcached.lcoal:11211) in Unknown on line 0 Of course I think we really need to get our session size down below that as much as we can, but we are not there yet, and who knows where else our session might be bloated. Googling around pointed to that being a hard limit in memcached, pointing to an old FAQ entry (e.g. http://stackoverflow.com/questions/5349/memcached-chunk-limit ) suggesting its a relatively hard limit. However, it looks like there is now an option for this! So I tried memcached server with -I 4194304 and indeed it fixes it. So there is dfinitely a way to get memached to work for our mamoth sessions. Some other comments: Note that from that transaction above you can see that the php driver handles the locking of the session Save handler is memcached not memcache save_path doesn't have tcp:// We probably need to think about the help texts. That sessionhandler text is quite long, maybe it be inappropiate to put some introductory text in the session handlers page? Similarly the setting this text seems redundant for each entry: 'for session information'
          Hide
          Petr Škoda added a comment -

          Hello,

          I guess this one should be considered legacy too, because:
          1/ it does not update fields in sessions table - we can not force logout (manual or after delete) or find out how many times is user logged in, etc.
          2/ the timeout logic is not handled in PHP - this may create major problems for SSO because you often one to synchronise the timeouts

          I think that the session drivers should have some static isLegacy() methods and the admin UI should warn admins that the drivers are not full features.

          Show
          Petr Škoda added a comment - Hello, I guess this one should be considered legacy too, because: 1/ it does not update fields in sessions table - we can not force logout (manual or after delete) or find out how many times is user logged in, etc. 2/ the timeout logic is not handled in PHP - this may create major problems for SSO because you often one to synchronise the timeouts I think that the session drivers should have some static isLegacy() methods and the admin UI should warn admins that the drivers are not full features.
          Hide
          Petr Škoda added a comment -

          The coding style recommends () after class constructor I think:

          $memcache_obj = new Memcache;
          

          and some ppl are obsessed with "_" in variable names (please note I actually like them).

          Show
          Petr Škoda added a comment - The coding style recommends () after class constructor I think: $memcache_obj = new Memcache; and some ppl are obsessed with "_" in variable names (please note I actually like them).
          Hide
          Petr Škoda added a comment -

          Hmm, how do we prevent memcached from discarding active sessions before timeout, it must have enough memory, right? How much is necessary?

          Show
          Petr Škoda added a comment - Hmm, how do we prevent memcached from discarding active sessions before timeout, it must have enough memory, right? How much is necessary?
          Hide
          Andrew Nicols added a comment -

          Something that I noticed about this when I was looking into it before (this may be what Dan was referring to above):
          https://github.com/timhunt/moodle/compare/master...MDL-31501#L4R816
          checks that the memcached extension is loaded, but then the memcache extension is actually used.

          I'd also prefer it if it were possible to drop additional session handlers in without changing core files - preferably in the same way that plugins are. I was also playing with redis (http://redis.io) which is a lot less mature than memcached but also has some potential for larger sites.

          Show
          Andrew Nicols added a comment - Something that I noticed about this when I was looking into it before (this may be what Dan was referring to above): https://github.com/timhunt/moodle/compare/master...MDL-31501#L4R816 checks that the memcached extension is loaded, but then the memcache extension is actually used. I'd also prefer it if it were possible to drop additional session handlers in without changing core files - preferably in the same way that plugins are. I was also playing with redis ( http://redis.io ) which is a lot less mature than memcached but also has some potential for larger sites.
          Hide
          Petr Škoda added a comment -

          Andrew: yes you can drop your session driver into /local/yoursession/ and from config.php override the session class by defining SESSION_CUSTOM_CLASS and SESSION_CUSTOM_FILE since 2.0.

          Show
          Petr Škoda added a comment - Andrew: yes you can drop your session driver into /local/yoursession/ and from config.php override the session class by defining SESSION_CUSTOM_CLASS and SESSION_CUSTOM_FILE since 2.0.
          Hide
          Petr Škoda added a comment -

          XMLDB editor - I think we could serialise the data to $CFG->dataroot/dev/ folder instead of session, I agree 100% we need to keep the session small.

          Show
          Petr Škoda added a comment - XMLDB editor - I think we could serialise the data to $CFG->dataroot/dev/ folder instead of session, I agree 100% we need to keep the session small.
          Hide
          Dan Poltawski added a comment -

          Aha, i'm glad I didn't complete my review and I decided to go to the beach because, Petr, you've responded in my absence!

          I guess this one should be considered legacy too, because:
          1/ it does not update fields in sessions table - we can not force logout (manual or after delete) or find out how many times is user logged in, etc.

          Is this documented anywhere? I think if I was an admin of a big site i'd go for this with the caveats in mind.

          2/ the timeout logic is not handled in PHP - this may create major problems for SSO because you often one to synchronise the timeouts

          Please can you explain more on this. I'm sure the type of sites who will want to use memcached will also likely be using SSO systems.

          Hmm, how do we prevent memcached from discarding active sessions before timeout, it must have enough memory, right? How much is necessary?

          We don't. That depends on the number of users etc. We need to leave memcached admin up to the admins, that is up to them to configure correctly. We should facilitate this not recommend it to novices.

          $memcache_obj = new Memcache;
          and some ppl are obsessed with "_" in variable names (please note I actually like them).

          I'm not obsessed by this, but I think avoiding _ in variable names (and encouraging in function names) makes it much easier to distinguish between function and variable names.

          Show
          Dan Poltawski added a comment - Aha, i'm glad I didn't complete my review and I decided to go to the beach because, Petr, you've responded in my absence! I guess this one should be considered legacy too, because: 1/ it does not update fields in sessions table - we can not force logout (manual or after delete) or find out how many times is user logged in, etc. Is this documented anywhere? I think if I was an admin of a big site i'd go for this with the caveats in mind. 2/ the timeout logic is not handled in PHP - this may create major problems for SSO because you often one to synchronise the timeouts Please can you explain more on this. I'm sure the type of sites who will want to use memcached will also likely be using SSO systems. Hmm, how do we prevent memcached from discarding active sessions before timeout, it must have enough memory, right? How much is necessary? We don't. That depends on the number of users etc. We need to leave memcached admin up to the admins, that is up to them to configure correctly. We should facilitate this not recommend it to novices. $memcache_obj = new Memcache; and some ppl are obsessed with "_" in variable names (please note I actually like them). I'm not obsessed by this, but I think avoiding _ in variable names (and encouraging in function names) makes it much easier to distinguish between function and variable names.
          Hide
          Petr Škoda added a comment -

          1/ the problem is that standard PHP sessions are simple key->value stores with optional locking with brute force timeouts, they do not know nothing about our users, auth plugins or complex SSO timeout synchronisations. In order to implement all requested functionally we have extra columns in the sessions table and we need this information to be up-to-date. I am going to write a file-base session driver that stores session info in database, locks on files and stores the actual session content in a directory to illustrate the difference between legacy and new session driver in Moodle.

          I personally think we should not encourage legacy drivers in admin UI at all. Unfortunately we have only one new driver for databases and the API itself needs some improvements. I'd really love to work on this in 2.4dev.

          2/ the standard session driver uses a callback to verify that session timeout is allowed, this helps with resolving of random SSO timeouts - grep for ignore_timeout_hook.

          Eh, why do variables start with '$' in PHP? To distinguish them from function names. This no-underscore-in-vars makesourcodeerrorpronebecauseyoucannotdistinguishwordsandtypethemproperly. The_underscores_are_often_usedWhenYouForSomeWeirdReasonDoNotUseCamelCase.

          Show
          Petr Škoda added a comment - 1/ the problem is that standard PHP sessions are simple key->value stores with optional locking with brute force timeouts, they do not know nothing about our users, auth plugins or complex SSO timeout synchronisations. In order to implement all requested functionally we have extra columns in the sessions table and we need this information to be up-to-date. I am going to write a file-base session driver that stores session info in database, locks on files and stores the actual session content in a directory to illustrate the difference between legacy and new session driver in Moodle. I personally think we should not encourage legacy drivers in admin UI at all. Unfortunately we have only one new driver for databases and the API itself needs some improvements. I'd really love to work on this in 2.4dev. 2/ the standard session driver uses a callback to verify that session timeout is allowed, this helps with resolving of random SSO timeouts - grep for ignore_timeout_hook. Eh, why do variables start with '$' in PHP? To distinguish them from function names. This no-underscore-in-vars makesourcodeerrorpronebecauseyoucannotdistinguishwordsandtypethemproperly. The_underscores_are_often_usedWhenYouForSomeWeirdReasonDoNotUseCamelCase.
          Hide
          Tim Hunt added a comment -

          Petr, if you get the chance to work on non-legacy file sessions, it would be great if you could work on non-legacy memcache sessions at the same time?

          Show
          Tim Hunt added a comment - Petr, if you get the chance to work on non-legacy file sessions, it would be great if you could work on non-legacy memcache sessions at the same time?
          Hide
          Petr Škoda added a comment -

          The trouble with memcache is locking, I would need something that automatically releases lock if the request is interrupted.

          But in general yes, once I start working on sessions I will try to create something for shared memory too.

          Show
          Petr Škoda added a comment - The trouble with memcache is locking, I would need something that automatically releases lock if the request is interrupted. But in general yes, once I start working on sessions I will try to create something for shared memory too.
          Hide
          Dan Poltawski added a comment -

          Well, I think i've done all my peer review with this, so changing the workflow.

          FWIW I think a good way forward with this would be to create a local plugin that admins can install with the options Petr talked about. Using a local plugin for session handling with config.php options does seem like a viable solution for admins who are competent to set up a memcached server to use. I also sort of agree its best not to expose this sort of thing as an admin option (simply because I think people should really know what they are doing, and not try randomly changing it, because then they will have to learn how to use config.php!)

          And on to the offtopic:

          Eh, why do variables start with '$' in PHP? To distinguish them from function names.

          $this->openfile;
          $this->open_file();
          
          Show
          Dan Poltawski added a comment - Well, I think i've done all my peer review with this, so changing the workflow. FWIW I think a good way forward with this would be to create a local plugin that admins can install with the options Petr talked about. Using a local plugin for session handling with config.php options does seem like a viable solution for admins who are competent to set up a memcached server to use. I also sort of agree its best not to expose this sort of thing as an admin option (simply because I think people should really know what they are doing, and not try randomly changing it, because then they will have to learn how to use config.php!) And on to the offtopic: Eh, why do variables start with '$' in PHP? To distinguish them from function names. $ this ->openfile; $ this ->open_file();
          Hide
          Petr Škoda added a comment -

          Ah, I forgot about properties, luckily when typing in PHPStorm the built-in autocomplete does not have problems with the differentiation. And later the "()" makes enough distinction for me personally. Anyway it this is a nice bikeshed...

          Show
          Petr Škoda added a comment - Ah, I forgot about properties, luckily when typing in PHPStorm the built-in autocomplete does not have problems with the differentiation. And later the "()" makes enough distinction for me personally. Anyway it this is a nice bikeshed...
          Hide
          Tony Levi added a comment -

          Hi guys,

          Some notes from our (recent) memcache implementation:

          1) IMPORTANT: the inbuilt session locking support of the php extension failed utterly for us, causing MASSIVE corruptions in quizzes and possibly other places. DO NOT rely on this, it is NOT SAFE.

          2) The current implementation here uses extra variables in memcache for the locking, but as Petr noted this means you do not get automatic expiry. Personally, I want to re-implement this to use advisory locks, with the storage being the only change from current db sessions. This also preserves the property that you can see which request / db connection is blocking other pages from running for a given session.

          3) The simplest way to avoid eviction of active sessions is to have enough memory which is easy; for our big sites there are 5 - 10k active sessions each and using less than 50MB per site.

          4) Course summaries in navigation was the main bloat point for sessions (users like to paste from MS Word so you get giant base64 chunks); we've cleaned this up and have gziped the data before it goes in any session handlers and since then there are no more error log entries showing too big sessions. I think we have submitted a patch for the course summary thing.

          I'm watching this issue now so if you have some questions that a big deployment(s) can answer, ask away - it would be much better for upstream to avoid the issues we had!!

          Show
          Tony Levi added a comment - Hi guys, Some notes from our (recent) memcache implementation: 1) IMPORTANT: the inbuilt session locking support of the php extension failed utterly for us, causing MASSIVE corruptions in quizzes and possibly other places. DO NOT rely on this, it is NOT SAFE. 2) The current implementation here uses extra variables in memcache for the locking, but as Petr noted this means you do not get automatic expiry. Personally, I want to re-implement this to use advisory locks, with the storage being the only change from current db sessions. This also preserves the property that you can see which request / db connection is blocking other pages from running for a given session. 3) The simplest way to avoid eviction of active sessions is to have enough memory which is easy; for our big sites there are 5 - 10k active sessions each and using less than 50MB per site. 4) Course summaries in navigation was the main bloat point for sessions (users like to paste from MS Word so you get giant base64 chunks); we've cleaned this up and have gziped the data before it goes in any session handlers and since then there are no more error log entries showing too big sessions. I think we have submitted a patch for the course summary thing. I'm watching this issue now so if you have some questions that a big deployment(s) can answer, ask away - it would be much better for upstream to avoid the issues we had!!
          Hide
          Petr Škoda added a comment -

          Tony: thanks a lot for the information!

          Show
          Petr Škoda added a comment - Tony: thanks a lot for the information!
          Hide
          Tim Hunt added a comment -

          Petr, you said that you hoped to get time to work on this for 2.4, so is it OK if I reassign this to you? If not, feel free to throw it back to me, or moodle.com. Thanks.

          Show
          Tim Hunt added a comment - Petr, you said that you hoped to get time to work on this for 2.4, so is it OK if I reassign this to you? If not, feel free to throw it back to me, or moodle.com. Thanks.
          Hide
          Petr Škoda added a comment -

          sure, I am looking forward to working on this

          Show
          Petr Škoda added a comment - sure, I am looking forward to working on this
          Hide
          Andrew Nicols added a comment -

          Has there been any movement on this? I assume it didn't make it into 2.4 since it's not closed, so I guess 2.5 is the new target?

          Show
          Andrew Nicols added a comment - Has there been any movement on this? I assume it didn't make it into 2.4 since it's not closed, so I guess 2.5 is the new target?
          Hide
          Matteo Scaramuccia added a comment -

          TNX to this Andrew's ping, I'd like to give my little contribution about can_work(): what about taking advices also from the MUC experience in finding the memcache* store ready? See a similar issue in MDL-36322 and the current proposal, not reviewed at the time of this comment, in https://github.com/scara/moodle/commit/07b8f49d1798e3fdebb6464eb10602d153115dde#L0R109. Andrew has already made mention of it, 08/Feb/12, and he told such an approach would give an extra perfomance hit here in the session framework code: maybe, this could be improved within the changes Petr was talking about the API.

          Show
          Matteo Scaramuccia added a comment - TNX to this Andrew's ping , I'd like to give my little contribution about can_work() : what about taking advices also from the MUC experience in finding the memcache* store ready? See a similar issue in MDL-36322 and the current proposal, not reviewed at the time of this comment, in https://github.com/scara/moodle/commit/07b8f49d1798e3fdebb6464eb10602d153115dde#L0R109 . Andrew has already made mention of it, 08/Feb/12 , and he told such an approach would give an extra perfomance hit here in the session framework code: maybe, this could be improved within the changes Petr was talking about the API.
          Hide
          Kris Stokking added a comment - - edited

          Moodlerooms is investigating a solution based on Redis that would allow for session locking as well as persistence.

          Show
          Kris Stokking added a comment - - edited Moodlerooms is investigating a solution based on Redis that would allow for session locking as well as persistence.
          Hide
          Andrew Nicols added a comment -

          Would be really good to see what you come up with Kris - I looked at this briefly when the OU put their memcached patch out there but didn't get a chance to finish it.

          Show
          Andrew Nicols added a comment - Would be really good to see what you come up with Kris - I looked at this briefly when the OU put their memcached patch out there but didn't get a chance to finish it.
          Hide
          Tim Hunt added a comment -

          Interesting warning about using memcache sessions: http://dormando.livejournal.com/495593.html

          Show
          Tim Hunt added a comment - Interesting warning about using memcache sessions: http://dormando.livejournal.com/495593.html
          Hide
          Kris Stokking added a comment - - edited

          Unfortunately, the PHPRedis extension we are using does not appear to natively support session locking. Although there does appear to be some community support for a custom session handler using Redis, it wouldn't perform as well and it could be a bit riskier to implement.

          However, it appears that the memcached PHP extension does support session locking according to its configuration manual. Has anyone already attempted session handling (with locking enabled) using the memcached PHP extension? We are about to switch gears from a Redis solution to one utilizing memcached, but I'm hoping we can avoid any redundancy if we're going down the wrong path.

          Edit: Fixing links

          Show
          Kris Stokking added a comment - - edited Unfortunately, the PHPRedis extension we are using does not appear to natively support session locking . Although there does appear to be some community support for a custom session handler using Redis , it wouldn't perform as well and it could be a bit riskier to implement. However, it appears that the memcached PHP extension does support session locking according to its configuration manual . Has anyone already attempted session handling (with locking enabled) using the memcached PHP extension? We are about to switch gears from a Redis solution to one utilizing memcached, but I'm hoping we can avoid any redundancy if we're going down the wrong path. Edit: Fixing links
          Hide
          Céline Perves added a comment -

          when this efficient solution should be integrated in Moodle versions?

          Show
          Céline Perves added a comment - when this efficient solution should be integrated in Moodle versions?
          Hide
          Martin Dougiamas added a comment -

          I'm going to bump this one up for the BACKEND crew to look at. A solution here could really help clustered servers quite a lot.

          Show
          Martin Dougiamas added a comment - I'm going to bump this one up for the BACKEND crew to look at. A solution here could really help clustered servers quite a lot.
          Hide
          Kris Stokking added a comment -

          Good timing - We're putting memcached sessions through QA testing now, and we should have the results back by mid-August at the latest. We'll confirm that locking and timeouts are working as expected. We'll be happy to post the code and configuration instructions once we've completed testing or if others are interested in getting involved.

          Show
          Kris Stokking added a comment - Good timing - We're putting memcached sessions through QA testing now, and we should have the results back by mid-August at the latest. We'll confirm that locking and timeouts are working as expected. We'll be happy to post the code and configuration instructions once we've completed testing or if others are interested in getting involved.
          Hide
          Martin Dougiamas added a comment -

          Superbly superb!

          Show
          Martin Dougiamas added a comment - Superbly superb!
          Hide
          Kris Stokking added a comment -

          Important Note: There is a bug in the memcached PHP client that causes sessions to persist indefinitely. In order for $CFG->sessiontimeout to be honored, the memcached extension must be at version 2.0.1 or greater. This can be checked by going to Server -> PHP info and looking at the Version under "memcached". For more information see https://bugs.php.net/bug.php?id=59641

          Logins, session data retrieval and session locking were working just fine in previous tests. We are currently re-testing everything against memcached extension version 2.1.0.

          Show
          Kris Stokking added a comment - Important Note: There is a bug in the memcached PHP client that causes sessions to persist indefinitely. In order for $CFG->sessiontimeout to be honored, the memcached extension must be at version 2.0.1 or greater. This can be checked by going to Server -> PHP info and looking at the Version under "memcached". For more information see https://bugs.php.net/bug.php?id=59641 Logins, session data retrieval and session locking were working just fine in previous tests. We are currently re-testing everything against memcached extension version 2.1.0.
          Hide
          Russell Smith added a comment -

          I think it's also worth saying that the "memcached" session handler is different from the "memcache" session handler. They are different implementations and will have different issues.

          Show
          Russell Smith added a comment - I think it's also worth saying that the "memcached" session handler is different from the "memcache" session handler. They are different implementations and will have different issues.
          Hide
          Kris Stokking added a comment -

          Good point Russell. We've successfully tested our patch, and we should either submit it here and change the title, or create a new ticket for memcached sessions and close this as Won't Fix (since memcache doesn't allow session locking). Martin - which would you prefer?

          Show
          Kris Stokking added a comment - Good point Russell. We've successfully tested our patch, and we should either submit it here and change the title, or create a new ticket for memcached sessions and close this as Won't Fix (since memcache doesn't allow session locking). Martin - which would you prefer?
          Hide
          Mark Nielsen added a comment -

          Howdy Folks, attaching our memcached_session class. It is pretty simple and any extra configurations you want to do to the memcached session handler you have to do in your php.ini or similar (See memcached configs).

          Obviously, this is just the session class, we need the other bits from Tim's patch to configure it and have Moodle switch to using it if configured to do so.

          Show
          Mark Nielsen added a comment - Howdy Folks, attaching our memcached_session class. It is pretty simple and any extra configurations you want to do to the memcached session handler you have to do in your php.ini or similar (See memcached configs ). Obviously, this is just the session class, we need the other bits from Tim's patch to configure it and have Moodle switch to using it if configured to do so.
          Hide
          Petr Škoda added a comment -

          Adding admin UI setting for session drivers seems very problematic to me because if the sessions stop working you can not revert the setting because you can not login, also if DB sessions do not work you can not even install moodle. So my +1 to have only config-dist.php help how to configure different session drivers. Also if we decide to modify anything in current session API we should migrate to automatic class loader to simplify using of custom session drivers that could be distributed as local plugins.

          Show
          Petr Škoda added a comment - Adding admin UI setting for session drivers seems very problematic to me because if the sessions stop working you can not revert the setting because you can not login, also if DB sessions do not work you can not even install moodle. So my +1 to have only config-dist.php help how to configure different session drivers. Also if we decide to modify anything in current session API we should migrate to automatic class loader to simplify using of custom session drivers that could be distributed as local plugins.
          Hide
          Russell Smith added a comment -

          Kris, I'm not sure your assertion that memcache doesn't support locking is true. http://www.matt-knight.co.uk/2011/concurrent-php-sessions/ and our experience of using suggests sessions do lock. However I'm not sure how that agrees with Tony's comments about session locking failing on them. Using the memcache session handler, you cannot load two moodle pages in separate tabs. One blocks until the first has loaded due to sessions.

          Show
          Russell Smith added a comment - Kris, I'm not sure your assertion that memcache doesn't support locking is true. http://www.matt-knight.co.uk/2011/concurrent-php-sessions/ and our experience of using suggests sessions do lock. However I'm not sure how that agrees with Tony's comments about session locking failing on them. Using the memcache session handler, you cannot load two moodle pages in separate tabs. One blocks until the first has loaded due to sessions.
          Hide
          Matthew Spurrier added a comment -

          Just a quick caution with using memcached for sessions: php by default does not write sessions until the page has completed processing , you really need to ensure that your initial page session is written to memcache as soon as possible, ie: using session_write_close to force the write.

          I have seen on many occasions race conditions occurring, where the session id has been written to the browser, then overridden by additional content requests also getting a different cookie.

          Show
          Matthew Spurrier added a comment - Just a quick caution with using memcached for sessions: php by default does not write sessions until the page has completed processing , you really need to ensure that your initial page session is written to memcache as soon as possible, ie: using session_write_close to force the write. I have seen on many occasions race conditions occurring, where the session id has been written to the browser, then overridden by additional content requests also getting a different cookie.
          Hide
          Mark Nielsen added a comment -

          Maybe I'm missing something, but isn't that the point of session locking? I feel like every session handler writes at the end or until your release the session by calling some function.

          Show
          Mark Nielsen added a comment - Maybe I'm missing something, but isn't that the point of session locking? I feel like every session handler writes at the end or until your release the session by calling some function.
          Hide
          Matthew Spurrier added a comment -

          The concern with race conditions is more when a page starts rendering prior to the script being finished, and including dynamic content generated by php (css/js/image etc), as well as considerations like jquery/ajax requests, iframe renderings, etc, which all have the potential over calling session_start prior to the initial request completing.

          Show
          Matthew Spurrier added a comment - The concern with race conditions is more when a page starts rendering prior to the script being finished, and including dynamic content generated by php (css/js/image etc), as well as considerations like jquery/ajax requests, iframe renderings, etc, which all have the potential over calling session_start prior to the initial request completing.
          Hide
          Martin Dougiamas added a comment -

          +1 for hijacking this to put the new Moodlerooms memcached code in so we can try it out!

          Show
          Martin Dougiamas added a comment - +1 for hijacking this to put the new Moodlerooms memcached code in so we can try it out!
          Hide
          Mark Nielsen added a comment -

          Updated ticket and pull information for memcached session handler. The previous values should be in the JIRA ticket history incase someone needs to find it for comparison, etc.

          The memcached session handler is the bare minimum and configuration requires only a single CFG in your Moodle config.php file.

          Flagged this for a documentation update so folks know that Moodle can use memcached for session handling if required.

          Show
          Mark Nielsen added a comment - Updated ticket and pull information for memcached session handler. The previous values should be in the JIRA ticket history incase someone needs to find it for comparison, etc. The memcached session handler is the bare minimum and configuration requires only a single CFG in your Moodle config.php file. Flagged this for a documentation update so folks know that Moodle can use memcached for session handling if required.
          Hide
          Dan Poltawski added a comment -

          Hi Guys - thanks for sharing the patch.

          Just briefly looking at it - I can't see the major differences in strategy between the Moodlerooms implementation of this and the OU one (https://github.com/danpoltawski/moodle/compare/MDL-31501). If anything, it is less further along in terms of configurability.

          But with both these patches, Petr and Tony's comments about the problems with the approach still stand? These are the problems we need to address to move this forward IMO.

          Show
          Dan Poltawski added a comment - Hi Guys - thanks for sharing the patch. Just briefly looking at it - I can't see the major differences in strategy between the Moodlerooms implementation of this and the OU one ( https://github.com/danpoltawski/moodle/compare/MDL-31501 ). If anything, it is less further along in terms of configurability. But with both these patches, Petr and Tony's comments about the problems with the approach still stand? These are the problems we need to address to move this forward IMO.
          Hide
          Mark Nielsen added a comment - - edited

          Hey Dan, AFAIK, the bulk of the OU code is to make it configurable through the UI, but then Petr voted for only being able to configuring it through the config.php so folks do not lock themselves out of their site. So, I ran with Petr's suggestion.

          In regards to Petr's suggestion about autoloading, this would be cool to have, but I don't have time to implement it that way and I also feel like having memcached session handling in core is better than having it as a local plugin. So, someone else can run with this if they like.

          Show
          Mark Nielsen added a comment - - edited Hey Dan, AFAIK, the bulk of the OU code is to make it configurable through the UI, but then Petr voted for only being able to configuring it through the config.php so folks do not lock themselves out of their site. So, I ran with Petr's suggestion. In regards to Petr's suggestion about autoloading, this would be cool to have, but I don't have time to implement it that way and I also feel like having memcached session handling in core is better than having it as a local plugin. So, someone else can run with this if they like.
          Hide
          Dan Poltawski added a comment -

          Sorry I was reffering to about the sessions being tracked in the database for logging out etc.

          Show
          Dan Poltawski added a comment - Sorry I was reffering to about the sessions being tracked in the database for logging out etc.
          Hide
          Kris Stokking added a comment -

          Dan - do you mean Petr's comment here?

          The questions that we're trying to address now aren't specific to memcache or memcached - they pertain to any session storage. From looking at the code, I can see that the database session implementation has added several "metadata" fields that couldn't possibly exist in any key/value pair system. Since KVP backends offer the best performance options, I feel that there is something wrong with expectations. This feeling is confirmed when I go back to look at the session code - I'm getting the impression that it's not legacy vs. standard sessions, but rather we're treating sessions using a relation DB as a special case. It would be very helpful if someone could point us in the direction of some docs that explain the difference between sessions and legacy sessions so that we can better understand the requirements.

          To address the questions more specifically:

          1. The only way to search for sessions in a KVP system is to ensure the session data contains all of the search fields we need and inspect every available session. It's certainly not pretty, but it would get the job done. Any session storage option could support this, and even if it was slow it would only be executed by administrators, and fairly infrequently at that I would imagine.

          2. The timeout logic could be handled by PHP, but is that the right approach? Sync'ing session timeouts between systems plagues SSO in general, and I'm not convinced that DB sessions has it right. Why are we simply ignoring the session timeout if any enabled authentication plugin tells us to? (That seems like a bug) We can add in the same support for ignoring session timeouts that DB sessions has, but we'll have to constantly update the session (as DB currently does) and we'll lose memcache's automatic garbage collection based on TTL.

          On a related note, Petr had brought up concerns around the release of session locks due to interruptions. In our testing, the lock was released properly and the session remained when the user hit an exception, a fatal error, and even when the server is restarted mid-execution.

          Show
          Kris Stokking added a comment - Dan - do you mean Petr's comment here ? The questions that we're trying to address now aren't specific to memcache or memcached - they pertain to any session storage. From looking at the code, I can see that the database session implementation has added several "metadata" fields that couldn't possibly exist in any key/value pair system. Since KVP backends offer the best performance options, I feel that there is something wrong with expectations. This feeling is confirmed when I go back to look at the session code - I'm getting the impression that it's not legacy vs. standard sessions, but rather we're treating sessions using a relation DB as a special case. It would be very helpful if someone could point us in the direction of some docs that explain the difference between sessions and legacy sessions so that we can better understand the requirements. To address the questions more specifically: 1. The only way to search for sessions in a KVP system is to ensure the session data contains all of the search fields we need and inspect every available session. It's certainly not pretty, but it would get the job done. Any session storage option could support this, and even if it was slow it would only be executed by administrators, and fairly infrequently at that I would imagine. 2. The timeout logic could be handled by PHP, but is that the right approach? Sync'ing session timeouts between systems plagues SSO in general, and I'm not convinced that DB sessions has it right. Why are we simply ignoring the session timeout if any enabled authentication plugin tells us to? (That seems like a bug) We can add in the same support for ignoring session timeouts that DB sessions has, but we'll have to constantly update the session (as DB currently does) and we'll lose memcache's automatic garbage collection based on TTL. On a related note, Petr had brought up concerns around the release of session locks due to interruptions. In our testing, the lock was released properly and the session remained when the user hit an exception, a fatal error, and even when the server is restarted mid-execution.
          Hide
          Martin Dougiamas added a comment -

          This issue is being bumped into the next sprint for BACKEND, aimed at 2.6

          Show
          Martin Dougiamas added a comment - This issue is being bumped into the next sprint for BACKEND, aimed at 2.6
          Hide
          Frédéric Massart added a comment -

          I am pushing this issue for peer review as I would like to get some feedback. Most of the current design is based on discussions with Petr Škoda.

          Remaining things to do:

          • Use a configurable lock. This requires MDL-25500 to offer it.
          • Possibly remove the abstract driver, to only have driver_standard as Moodle is not supposed to work without the database table (dixit Petr).
            • Then the old session_stub would be restored where it was, but deprecated.
            • The old file_session would be restored where it was as well.
          • Calling gc() on the storages.

          The commits are really granular so that is demonstrates that I haven't really modified anything in database_session, which is required if we want to make sure everything will work as it was before.

          Thanks!
          Fred

          Show
          Frédéric Massart added a comment - I am pushing this issue for peer review as I would like to get some feedback. Most of the current design is based on discussions with Petr Škoda . Remaining things to do: Use a configurable lock. This requires MDL-25500 to offer it. Possibly remove the abstract driver, to only have driver_standard as Moodle is not supposed to work without the database table (dixit Petr). Then the old session_stub would be restored where it was, but deprecated. The old file_session would be restored where it was as well. Calling gc() on the storages. The commits are really granular so that is demonstrates that I haven't really modified anything in database_session, which is required if we want to make sure everything will work as it was before. Thanks! Fred
          Hide
          Mark Nielsen added a comment - - edited

          Some questions:

          1. Can you please explain at a high level what is happening now?
          2. Why are we moving back to memcache instead of memcached?
          3. How is the performance on this new code? The main reason for the memcached session handler is for performance and it handles locking and sessions natively, in C code. It looks like the new code is roughly the same as before but the session data goes to memcache instead of the database. Perhaps that was the bottleneck? Not sure.

          Thanks, Cheers, Mark

          Show
          Mark Nielsen added a comment - - edited Some questions: Can you please explain at a high level what is happening now? Why are we moving back to memcache instead of memcached? How is the performance on this new code? The main reason for the memcached session handler is for performance and it handles locking and sessions natively, in C code. It looks like the new code is roughly the same as before but the session data goes to memcache instead of the database. Perhaps that was the bottleneck? Not sure. Thanks, Cheers, Mark
          Hide
          Martin Dougiamas added a comment -

          +1 for explanations, Fred and Petr

          Show
          Martin Dougiamas added a comment - +1 for explanations, Fred and Petr
          Hide
          Frédéric Massart added a comment - - edited

          1/ The only difference with the current system is that the session data is stored in an external storage, as you noticed.
          2/ No idea, I picked that one because it's more commonly available to distributions, however we can either add a new memcached storage, or add one character to this one.
          3/ I am not sure how to measure this, I guess it all depends on the availability of the storage that was set up, and the size of the session data. Still, it seems that we could save a DB query when using an external storage.

          As per my understanding, the sessions have to use the database for multiple reasons:

          • Moodle needs to know all the existing sessions at time, to be able to kill one or all. The sessions opened is not an information disclosed by PHP. However it looks like we could use the advanced features of Memcached to fetch the keys from the server and do our magic, but at this stage that would be highly experimental.
          • We need to have a hand on the session timeout, by using PHP we cannot cancel a session timeout. This feature is useful for MNet or SSO.
          • We need a reliable locking system, I'm afraid I can't comment on the reliability of the one offered by PHP.

          Petr Škoda has certainly a better insight on all of this.

          Show
          Frédéric Massart added a comment - - edited 1/ The only difference with the current system is that the session data is stored in an external storage, as you noticed. 2/ No idea, I picked that one because it's more commonly available to distributions, however we can either add a new memcached storage, or add one character to this one. 3/ I am not sure how to measure this, I guess it all depends on the availability of the storage that was set up, and the size of the session data. Still, it seems that we could save a DB query when using an external storage. As per my understanding, the sessions have to use the database for multiple reasons: Moodle needs to know all the existing sessions at time, to be able to kill one or all. The sessions opened is not an information disclosed by PHP. However it looks like we could use the advanced features of Memcached to fetch the keys from the server and do our magic, but at this stage that would be highly experimental. We need to have a hand on the session timeout, by using PHP we cannot cancel a session timeout. This feature is useful for MNet or SSO. We need a reliable locking system, I'm afraid I can't comment on the reliability of the one offered by PHP. Petr Škoda has certainly a better insight on all of this.
          Hide
          Martin Dougiamas added a comment - - edited

          From all the quick research I did it seems the memcached client library is more recent and maintained, so I would think we can forget about memcache. Being in distributions is not really a factor. Anyone with a site large enough to need this sort of thing knows how to install a package.

          I think we really need to have some performance comparisons here on a large system with heavy use:

          • current database sessions
          • Moodlerooms patch fully using memcached
          • Fred's patch which mixes Moodle db and memcached

          If the performance of Fred's patch is significantly worse than Moodlerooms then we really need very very good answers to these questions:

          • why/when does Moodle need to kill sessions, can't it just start new ones? What is the danger there?
          • why can't we take advantage of the native locking system that I guess a lot of people use?
          Show
          Martin Dougiamas added a comment - - edited From all the quick research I did it seems the memcached client library is more recent and maintained, so I would think we can forget about memcache. Being in distributions is not really a factor. Anyone with a site large enough to need this sort of thing knows how to install a package. I think we really need to have some performance comparisons here on a large system with heavy use: current database sessions Moodlerooms patch fully using memcached Fred's patch which mixes Moodle db and memcached If the performance of Fred's patch is significantly worse than Moodlerooms then we really need very very good answers to these questions: why/when does Moodle need to kill sessions, can't it just start new ones? What is the danger there? why can't we take advantage of the native locking system that I guess a lot of people use?
          Hide
          Kris Stokking added a comment -

          There are a lot of good comments in this ticket that are getting overlooked. Rewriting the patch to use memcached instead of memcache had nothing to do with distributions, but rather with the fact that session locking was not working for Moodlerooms or for NetSpot, which would potentially cause corruption of session data. I have no idea why it's not working - it should in theory - so it could possibly be an issue with configuration or it could be in the implementation of the memcache PHP client. Russell Smith had success with locking using memcache, perhaps he could give more insight into his environment (which version was used for the PHP client and server? Were sticky sessions used?). Until we can prove that it is in fact working as expected, I'd recommend that we exclude memcache.

          Regarding locking - Moodlerooms has tested and reported that the out-of-the-box locking that is available with the PHP Memcached client works as expected, including cases where a page or the server has died. We've posted that to the ticket. If we need more details here, we're happy to provide them.

          Regarding performance - Memcached is not just another storage option for sessions - it is highly performant for key lookups because they are done in memory, avoiding the query optimizations and contentions that are present in the RDBMS and cause slowness and scalability issues. I'm very concerned that we are trying to over-engineer sessions, and that is counter to the performance goals of this ticket. If we require that that sessions have to use the database (as has been stated above), then we have completely invalidated the goal of using a highly performant backend for session storage because we're putting slow calls to the database in-front of fast calls to memcached for session storage. I'm not even sure what the benefits of using memcached as the session store would be in that case.

          Show
          Kris Stokking added a comment - There are a lot of good comments in this ticket that are getting overlooked. Rewriting the patch to use memcached instead of memcache had nothing to do with distributions, but rather with the fact that session locking was not working for Moodlerooms or for NetSpot, which would potentially cause corruption of session data. I have no idea why it's not working - it should in theory - so it could possibly be an issue with configuration or it could be in the implementation of the memcache PHP client. Russell Smith had success with locking using memcache, perhaps he could give more insight into his environment (which version was used for the PHP client and server? Were sticky sessions used?). Until we can prove that it is in fact working as expected, I'd recommend that we exclude memcache. Regarding locking - Moodlerooms has tested and reported that the out-of-the-box locking that is available with the PHP Memcached client works as expected, including cases where a page or the server has died. We've posted that to the ticket. If we need more details here, we're happy to provide them. Regarding performance - Memcached is not just another storage option for sessions - it is highly performant for key lookups because they are done in memory, avoiding the query optimizations and contentions that are present in the RDBMS and cause slowness and scalability issues. I'm very concerned that we are trying to over-engineer sessions, and that is counter to the performance goals of this ticket. If we require that that sessions have to use the database (as has been stated above), then we have completely invalidated the goal of using a highly performant backend for session storage because we're putting slow calls to the database in-front of fast calls to memcached for session storage. I'm not even sure what the benefits of using memcached as the session store would be in that case.
          Hide
          Petr Škoda added a comment -

          We are not overengineering sessions, we need the extra features to implement existing functionality such as listing of active user sessions (used in add-ons), forced logout (upgrades, deleting and suspending of users) and synchronised session timeouts with external systems (SSO). If you do not like that use the current drivers such as legacy files and moodlerooms/OU memcahe - it will be still available.

          This issue is about new full featured session driver architecture, please let's stop discussing legacy drivers (using the basic built in PHP session support) here.

          The only question is if we should distribute more legacy drivers in standard distribution, my -1 for that because those features described above can not work with them, but that should be a separate issue imho.

          Show
          Petr Škoda added a comment - We are not overengineering sessions, we need the extra features to implement existing functionality such as listing of active user sessions (used in add-ons), forced logout (upgrades, deleting and suspending of users) and synchronised session timeouts with external systems (SSO). If you do not like that use the current drivers such as legacy files and moodlerooms/OU memcahe - it will be still available. This issue is about new full featured session driver architecture, please let's stop discussing legacy drivers (using the basic built in PHP session support) here. The only question is if we should distribute more legacy drivers in standard distribution, my -1 for that because those features described above can not work with them, but that should be a separate issue imho.
          Hide
          Martin Dougiamas added a comment - - edited

          So, the reasons for the extra data are:

          1. listing of active user sessions (used in add-ons)
          2. forced logout (upgrades, deleting and suspending of users)
          3. synchronised session timeouts with external systems (SSO)

          I can think of a lot of sites that don't need these and would happily make the tradeoff for increased speed. Can we perhaps stop thinking of these as requirements and instead make them features of particular session backends?

          If it was documented well besides the config-dist.php settings to switch session backends then admins can make informed choices.

          Show
          Martin Dougiamas added a comment - - edited So, the reasons for the extra data are: 1. listing of active user sessions (used in add-ons) 2. forced logout (upgrades, deleting and suspending of users) 3. synchronised session timeouts with external systems (SSO) I can think of a lot of sites that don't need these and would happily make the tradeoff for increased speed. Can we perhaps stop thinking of these as requirements and instead make them features of particular session backends? If it was documented well besides the config-dist.php settings to switch session backends then admins can make informed choices.
          Hide
          Russell Smith added a comment -

          Currently we just use RHEL package (php-pecl-memcache-3.0.5-4.el6.x86_64)

          Config is;
          $CFG->sess_memcache_hosts = 'tcp://memcache1.com:11211?retry_interval=300, tcp://memcache2.com:11211?retry_interval=300';
          define('SESSION_CUSTOM_CLASS', 'memcache_session');
          define('SESSION_CUSTOM_FILE', '/local/memcache_session.php');
          ini_set('memcache.session_redundancy', 3);

          $CFG->memcache_cfg = true;
          $CFG->memcache_hosts = 'tcp://memcache1.com:11211?retry_interval=300, tcp://memcache2.com:11211?retry_interval=300';
          $CFG->memcache_cfg_ttl = 60;
          $CFG->memcache_cfg_prefix = 'PRD';

          We use a version much like the one posted here for memcache. With this configuration we can successfully remove, patch and reinstate a memcache node with zero logouts of users. We've not see any session corruption issues and the pages will not load until a previous page has finished.

          Show
          Russell Smith added a comment - Currently we just use RHEL package (php-pecl-memcache-3.0.5-4.el6.x86_64) Config is; $CFG->sess_memcache_hosts = 'tcp://memcache1.com:11211?retry_interval=300, tcp://memcache2.com:11211?retry_interval=300'; define('SESSION_CUSTOM_CLASS', 'memcache_session'); define('SESSION_CUSTOM_FILE', '/local/memcache_session.php'); ini_set('memcache.session_redundancy', 3); $CFG->memcache_cfg = true; $CFG->memcache_hosts = 'tcp://memcache1.com:11211?retry_interval=300, tcp://memcache2.com:11211?retry_interval=300'; $CFG->memcache_cfg_ttl = 60; $CFG->memcache_cfg_prefix = 'PRD'; We use a version much like the one posted here for memcache. With this configuration we can successfully remove, patch and reinstate a memcache node with zero logouts of users. We've not see any session corruption issues and the pages will not load until a previous page has finished.
          Hide
          Russell Smith added a comment -

          In regards to trade-offs and database use. I don't think it's overwhelming to update the database on certain operations. Especially login and logout to update a current sessions table. Most of the performance advantage is not having to connect and send/receive data for each page load. Writing to the database for each page load is a large overhead.

          I would think the design could allow all the features to be used with minimal impact on database usage and therefore performance.

          The sticking point here is locking. Using the db for locking can be slow. There is little to no trust in the locking provided by PHP session management for memcache which means we need to lock somewhere else. My current understanding is that is where all the performance hit and time will be spent. But providing well performing solutions for that will be difficult. Especially if we can't use the memcache(d) native locking implementations.

          Show
          Russell Smith added a comment - In regards to trade-offs and database use. I don't think it's overwhelming to update the database on certain operations. Especially login and logout to update a current sessions table. Most of the performance advantage is not having to connect and send/receive data for each page load. Writing to the database for each page load is a large overhead. I would think the design could allow all the features to be used with minimal impact on database usage and therefore performance. The sticking point here is locking. Using the db for locking can be slow. There is little to no trust in the locking provided by PHP session management for memcache which means we need to lock somewhere else. My current understanding is that is where all the performance hit and time will be spent. But providing well performing solutions for that will be difficult. Especially if we can't use the memcache(d) native locking implementations.
          Hide
          Mark Nielsen added a comment -

          2. forced logout (upgrades, deleting and suspending of users)

          Just a suggestion, this would logout all users, but you can change the session key prefix in memcached via a ini setting. You could increment this or randomize it whenever needed. Unfortunately, the prefix might add some overhead itself - maybe a CFG and CFG gets stored in MUC? This wouldn't allow you to invalidate individual sessions though or a group of users.

          From Martin Dougiamas

          I can think of a lot of sites that don't need these and would happily make the tradeoff for increased speed. Can we perhaps stop thinking of these as requirements and instead make them features of particular session backends?

          +1 here. We could potentially configure clients to the session driver that makes sense for their authentication policies.

          Show
          Mark Nielsen added a comment - 2. forced logout (upgrades, deleting and suspending of users) Just a suggestion, this would logout all users, but you can change the session key prefix in memcached via a ini setting. You could increment this or randomize it whenever needed. Unfortunately, the prefix might add some overhead itself - maybe a CFG and CFG gets stored in MUC? This wouldn't allow you to invalidate individual sessions though or a group of users. From Martin Dougiamas I can think of a lot of sites that don't need these and would happily make the tradeoff for increased speed. Can we perhaps stop thinking of these as requirements and instead make them features of particular session backends? +1 here. We could potentially configure clients to the session driver that makes sense for their authentication policies.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks everyone! I have added some commits to the patch to add the custom Memcached driver, as it seems that we would unofficially support it. However, I noticed a few problems that still need to be fixed. The methods quit() and touch() of Memcached are only available from PECL 2, which I do not have. Also, should we use a persistent connexion between request as the cache store does?

          In regard with the new methods kill_user() and gc(), I'm sure we could find a workaround to have a fully featured Memcached driver.

          Show
          Frédéric Massart added a comment - - edited Thanks everyone! I have added some commits to the patch to add the custom Memcached driver, as it seems that we would unofficially support it. However, I noticed a few problems that still need to be fixed. The methods quit() and touch() of Memcached are only available from PECL 2, which I do not have. Also, should we use a persistent connexion between request as the cache store does? In regard with the new methods kill_user() and gc(), I'm sure we could find a workaround to have a fully featured Memcached driver.
          Hide
          Frédéric Massart added a comment - - edited

          I am re-requesting a peer review on this.

          The current patch provides an unofficial support for Memcached session handler. It clearly does not support all the callbacks that it should, but can probably fit the needs of some.

          However, this highlights some issues such as the deletion of a user, in which case the sessions associated to it cannot be killed. Also killing all the sessions would not work depending on the installed version of the Memcached extension.

          Please let me know what you think of this.

          Edit: If we agree on this implementation, I suppose it is not necessary to support different storages for the database driver.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited I am re-requesting a peer review on this. The current patch provides an unofficial support for Memcached session handler. It clearly does not support all the callbacks that it should, but can probably fit the needs of some. However, this highlights some issues such as the deletion of a user, in which case the sessions associated to it cannot be killed. Also killing all the sessions would not work depending on the installed version of the Memcached extension. Please let me know what you think of this. Edit: If we agree on this implementation, I suppose it is not necessary to support different storages for the database driver. Cheers, Fred
          Hide
          Petr Škoda added a comment - - edited

          Hi, I looked at the code and I kept thinking about the performance, I like your patch. My worry is that the performance is going to be still relatively low compared to non-locked 1.x DB sessions or legacy memcache(d) driver that does not update the sessions table. I am afraid we will have to trade some reliability for performance, there are a few locking strategies we could present to admins in site settings or config.php:

          1/ No session locking at all - there were many sites taking the chances in 1.x, I guess public static sites ( === few changes of structure and permissions) like moodle.org or learn.moodle.net could take the risk, both perceived and absolute performance should be significantly better. Also for quite some time there was a nasty bug that was actually increasing the probability of session overriding in early 2.x and people did not complain much. I would personally expect a lot more nasty problems to pop-up, but somehow they did not get reported...
          2/ Partial session locking in critical modules/areas - such as quiz tests, the idea is to introduce two methods in session driver - require_locking() + no_locking_requited_any_more() and add + set a new flag in session.lockingrequired DB table column. This would be used in places where students can not logout/login again in case of weird session problems. Hopefully this could be relatively safe for large production sites if we pinpoint the critical areas. Fewer locks === better performance in general.
          3/ Require locking for non-guest accounts only (this is already implemented). Safe for production sites imo.
          4/ Require locking for everybody - for troubleshooting only.
          5/ We could also require locking for specified accounts only - troubleshooting too I guess.

          Great job, thanks! I am very glad there are more people that understand the session internals now.

          Show
          Petr Škoda added a comment - - edited Hi, I looked at the code and I kept thinking about the performance, I like your patch. My worry is that the performance is going to be still relatively low compared to non-locked 1.x DB sessions or legacy memcache(d) driver that does not update the sessions table. I am afraid we will have to trade some reliability for performance, there are a few locking strategies we could present to admins in site settings or config.php: 1/ No session locking at all - there were many sites taking the chances in 1.x, I guess public static sites ( === few changes of structure and permissions) like moodle.org or learn.moodle.net could take the risk, both perceived and absolute performance should be significantly better. Also for quite some time there was a nasty bug that was actually increasing the probability of session overriding in early 2.x and people did not complain much. I would personally expect a lot more nasty problems to pop-up, but somehow they did not get reported... 2/ Partial session locking in critical modules/areas - such as quiz tests, the idea is to introduce two methods in session driver - require_locking() + no_locking_requited_any_more() and add + set a new flag in session.lockingrequired DB table column. This would be used in places where students can not logout/login again in case of weird session problems. Hopefully this could be relatively safe for large production sites if we pinpoint the critical areas. Fewer locks === better performance in general. 3/ Require locking for non-guest accounts only (this is already implemented). Safe for production sites imo. 4/ Require locking for everybody - for troubleshooting only. 5/ We could also require locking for specified accounts only - troubleshooting too I guess. Great job, thanks! I am very glad there are more people that understand the session internals now.
          Hide
          Kris Stokking added a comment -

          I would completely agree with Russell's comment about storing certain bits of data in the database at important events, like login. If we were to split the current mdl_sessions table in two such that the userid, timecreated and sessid fields were stored for all session drivers, but only the database (or standard) one used the state, timemodified, sessdata, etc fields for storage, then we could track all open sessions in order to kill them for a specific user. And also for garbage collection - just compare the timecreated field to the user's lastaccess + session timeout, buffered by some number (e.g. a day). In the case of memcached, we'd also check to see if the record is still available in the case of a long-running page (if it wasn't accessed, it would be kicked out based on the TTL). That would be a huge benefit to memcached and file sessions with a hit that would be barely noticeable.

          Some other notables:

          Russell - I would disagree with your comment about trusting the out-of-the-box locking. We could potentially build in the option to add an extra layer of locking, which may be useful in some cases, but I trust memcached and you trust memcache, so why not use that to our advantage?

          Frederic - I would highly recommend that we require memcached version 2.1.0, which has been out since August of 2012. It avoids having to deal with a nasty bug, which I commented on here. It also allows us to use all of the available feature set (e.g. quit and touch). I think you raise a good question about session persistence, but I would lean against its implementation.

          Petr - I'm not sure I understand your post. The performance of what, specifically, is still going to be relatively low? The reason database sessions are so slow (comparatively speaking) is because you have to implement the expensive locking and timeout functionality in the driver for every read and write, whereas they are natively handled in the PECL extension for memcached. The way to improve performance is to switch to memcached. I don't think that we should be presenting options to admins on how to fine tune locking, because they won't truly understand their implications until they've hit a problem.

          Show
          Kris Stokking added a comment - I would completely agree with Russell's comment about storing certain bits of data in the database at important events, like login. If we were to split the current mdl_sessions table in two such that the userid, timecreated and sessid fields were stored for all session drivers, but only the database (or standard) one used the state, timemodified, sessdata, etc fields for storage, then we could track all open sessions in order to kill them for a specific user. And also for garbage collection - just compare the timecreated field to the user's lastaccess + session timeout, buffered by some number (e.g. a day). In the case of memcached, we'd also check to see if the record is still available in the case of a long-running page (if it wasn't accessed, it would be kicked out based on the TTL). That would be a huge benefit to memcached and file sessions with a hit that would be barely noticeable. Some other notables: Russell - I would disagree with your comment about trusting the out-of-the-box locking. We could potentially build in the option to add an extra layer of locking, which may be useful in some cases, but I trust memcached and you trust memcache, so why not use that to our advantage? Frederic - I would highly recommend that we require memcached version 2.1.0, which has been out since August of 2012. It avoids having to deal with a nasty bug, which I commented on here . It also allows us to use all of the available feature set (e.g. quit and touch). I think you raise a good question about session persistence, but I would lean against its implementation. Petr - I'm not sure I understand your post. The performance of what, specifically, is still going to be relatively low? The reason database sessions are so slow (comparatively speaking) is because you have to implement the expensive locking and timeout functionality in the driver for every read and write, whereas they are natively handled in the PECL extension for memcached. The way to improve performance is to switch to memcached. I don't think that we should be presenting options to admins on how to fine tune locking, because they won't truly understand their implications until they've hit a problem.
          Hide
          Eric Merrill added a comment -

          To me, the big part of this is not doing a DB read and write (and associated locking) on every page load. That doesn't mean we can never store something in the DB. I'm with Kris on this - I would just store the sessionid (and userid) on login and then do cleanup periodically (ie cron). This makes the session available for administrative operations. Assuming memcached's TTL is working correctly, you could just do your garbage collection of the table just by checking if the session id still exists in memcached (unless I'm missing something there).

          I also think that recent memcacheds' are the way to go from the research I've done - it seems like it has the most native features what we need.

          Also, it seems like exposing locking changes, even to admins, is a bit risky - I'm a pretty experience administrator, and have been programming in Moodle for years, but I still don't fully grasp the issues around locking and its existence. I feel like it would be so tempting for admins who just see there is a potential performance improvement, without really grasping the tradeoff.

          That's my 2 cents, even if it's probably only worth half that.

          Show
          Eric Merrill added a comment - To me, the big part of this is not doing a DB read and write (and associated locking) on every page load. That doesn't mean we can never store something in the DB. I'm with Kris on this - I would just store the sessionid (and userid) on login and then do cleanup periodically (ie cron). This makes the session available for administrative operations. Assuming memcached's TTL is working correctly, you could just do your garbage collection of the table just by checking if the session id still exists in memcached (unless I'm missing something there). I also think that recent memcacheds' are the way to go from the research I've done - it seems like it has the most native features what we need. Also, it seems like exposing locking changes, even to admins, is a bit risky - I'm a pretty experience administrator, and have been programming in Moodle for years, but I still don't fully grasp the issues around locking and its existence. I feel like it would be so tempting for admins who just see there is a potential performance improvement, without really grasping the tradeoff. That's my 2 cents, even if it's probably only worth half that.
          Hide
          Petr Škoda added a comment -

          Please let's not complicate things - the native PHP drivers (aka moodle legacy session drivers) are not affected by this issue at all, they will continue working as before and you can do whatever you want there, some features can not work with them, I hope it is now clear.

          This issue is about adding a bit more general moodle session driver written in PHP. Could we please stop discussing legacy drivers here and instead concentrate on the architecture and performance of the new session driver? If necessary please create a new issue for legacy driver hacks.

          Show
          Petr Škoda added a comment - Please let's not complicate things - the native PHP drivers (aka moodle legacy session drivers) are not affected by this issue at all, they will continue working as before and you can do whatever you want there, some features can not work with them, I hope it is now clear. This issue is about adding a bit more general moodle session driver written in PHP. Could we please stop discussing legacy drivers here and instead concentrate on the architecture and performance of the new session driver? If necessary please create a new issue for legacy driver hacks.
          Hide
          Martin Dougiamas added a comment - - edited

          +1 for requiring memcached 2.1.0 or later to avoid that timeout bug ... I think anyone setting up this sort of stuff will not find this sort of restriction a problem.

          -1 for options on bringing in the fine-tuning of locking in this issue. I would implement this as is for 2.6, but make sure that help is available (in config-dist.php with link to a page in Moodle Docs) explaining that certain minor features in Moodle (ending sessions for deleted users etc) may not work correctly. The Docs page can be used by the community to document all of these minor features and any potential gotchas so the admin choosing Memcached can make a decision based on the tradeoffs. I know they would not affect moodle.org, for example... we'd use memcached sessions immediately and enjoy the increased performance.

          In future version another issue can solve these missing minor features (with new tables etc) in a way that does not reduce performance much, but I think that is out of scope for this issue.

          Show
          Martin Dougiamas added a comment - - edited +1 for requiring memcached 2.1.0 or later to avoid that timeout bug ... I think anyone setting up this sort of stuff will not find this sort of restriction a problem. -1 for options on bringing in the fine-tuning of locking in this issue. I would implement this as is for 2.6, but make sure that help is available (in config-dist.php with link to a page in Moodle Docs) explaining that certain minor features in Moodle (ending sessions for deleted users etc) may not work correctly. The Docs page can be used by the community to document all of these minor features and any potential gotchas so the admin choosing Memcached can make a decision based on the tradeoffs. I know they would not affect moodle.org, for example... we'd use memcached sessions immediately and enjoy the increased performance. In future version another issue can solve these missing minor features (with new tables etc) in a way that does not reduce performance much, but I think that is out of scope for this issue.
          Hide
          Russell Smith added a comment -

          Given the profile on this, and it being the first non-database session module I think it is important that it provides the full functionality of the session library. This driver will be used as a model to write different session handlers. If it's not done right here, half done session handlers will become more widespread. People will expect they will be fully featured as it implements the same features as memcached handler. Documentation on the wiki of the caveats is not really a solution.

          What is so difficult about making it perform all the currently available functions? That needs explaining, as it will need to be completed in a linked issue even if it's agreed to defer it.

          Those who see it as critically important to keep the current performance can run the legacy handler. If you upgrade the capabilities in another issue those who were using this version will expect the same performance level. It will not be easy to implement the rest of the features as they will degrade performance.

          Show
          Russell Smith added a comment - Given the profile on this, and it being the first non-database session module I think it is important that it provides the full functionality of the session library. This driver will be used as a model to write different session handlers. If it's not done right here, half done session handlers will become more widespread. People will expect they will be fully featured as it implements the same features as memcached handler. Documentation on the wiki of the caveats is not really a solution. What is so difficult about making it perform all the currently available functions? That needs explaining, as it will need to be completed in a linked issue even if it's agreed to defer it. Those who see it as critically important to keep the current performance can run the legacy handler. If you upgrade the capabilities in another issue those who were using this version will expect the same performance level. It will not be easy to implement the rest of the features as they will degrade performance.
          Hide
          Martin Dougiamas added a comment -

          Kris just went over this idea of the table with the sess id etc and actually, yes, it's not as difficult as I'd thought above. Fred, I think it's a good idea.

          Show
          Martin Dougiamas added a comment - Kris just went over this idea of the table with the sess id etc and actually, yes, it's not as difficult as I'd thought above. Fred, I think it's a good idea.
          Hide
          Petr Škoda added a comment - - edited

          Let me try to explain the source of confusion again:

          1/ standard moodle session driver (current database sessions and proposed patch above) are implemented using session_set_save_handler() user-level session storage functions, we control EVERYTHING in Moodle PHP code, all technical problems can be resolved easily usually by adding new fields to the session table, performance is lower because PHP does not have reliable/scalable locking mechanism

          2/ legacy session drivers (current file session, memcached from OU and MR) are implemented as a black box using ini_set('session.save_handler', 'memcached') or ini_set('session.save_handler', 'files'), we have zero control over session expiration, we can not list active sessions, we do not control locking, etc. Moodle 1.x was using this session mechanism.

          There is a big difference between these two approaches, I believe we should discuss and implement them separately.

          Show
          Petr Škoda added a comment - - edited Let me try to explain the source of confusion again: 1/ standard moodle session driver (current database sessions and proposed patch above) are implemented using session_set_save_handler() user-level session storage functions, we control EVERYTHING in Moodle PHP code, all technical problems can be resolved easily usually by adding new fields to the session table, performance is lower because PHP does not have reliable/scalable locking mechanism 2/ legacy session drivers (current file session, memcached from OU and MR) are implemented as a black box using ini_set('session.save_handler', 'memcached') or ini_set('session.save_handler', 'files'), we have zero control over session expiration, we can not list active sessions, we do not control locking, etc. Moodle 1.x was using this session mechanism. There is a big difference between these two approaches, I believe we should discuss and implement them separately.
          Hide
          Kris Stokking added a comment -

          If you want to create 2 plugins, one for "standard memcached" and another for "legacy memcached" because you want more control over session handling, that's technically fine but I see no benefits to doing so. You haven't listed any use case that legacy memcached can't support. All we need is a minor change to the behavior of mdl_sessions and the legacy memcached driver will be both functionally equivalent as well as outperform the standard version.

          • Sessions expiration - handled natively by the PECL extension for memcached. Also, we can evict sessions based on data we have collected on login only (userid, sessid and timecreated)
          • List active sessions - we can do this based on the data we have collected on login only. We may need to check the sessions to see if they're still active, or have some flexibility in the accuracy of the report, but that's certainly not a reason to sacrifice performance
          • Locking - handled natively by the PECL extension for memcached

          I'm happy to walk you through the design outside of this thread since we're just going around in circles at this point.

          Show
          Kris Stokking added a comment - If you want to create 2 plugins, one for "standard memcached" and another for "legacy memcached" because you want more control over session handling, that's technically fine but I see no benefits to doing so. You haven't listed any use case that legacy memcached can't support. All we need is a minor change to the behavior of mdl_sessions and the legacy memcached driver will be both functionally equivalent as well as outperform the standard version. Sessions expiration - handled natively by the PECL extension for memcached. Also, we can evict sessions based on data we have collected on login only (userid, sessid and timecreated) List active sessions - we can do this based on the data we have collected on login only. We may need to check the sessions to see if they're still active, or have some flexibility in the accuracy of the report, but that's certainly not a reason to sacrifice performance Locking - handled natively by the PECL extension for memcached I'm happy to walk you through the design outside of this thread since we're just going around in circles at this point.
          Hide
          Petr Škoda added a comment - - edited

          No. PHP save_handler sidesteps all our PHP code in session driver, it is really very different. Your memcached driver using save_handler fails badly for anything that needs to interfere with session timeouts for example.

          1/ You do not control the session expiration done by save_handler apart from setting minimal timeout before the session gets discarded. SSO needs a lot more control over expiration of sessions, they must not just disappear. Really this can not be done with memcached or file save_handler. Functions registered via session_set_save_handler() can do anything you can imagine with the session timeouts (for example different timeouts for different account types).

          2/ Yes, you can emulate the list of active sessions, but you need extra validation in session storage because thanks to 1/ you can not trust the contents of your emulated sessions table.

          3/ When using save_handler the locking is all or nothing, again you do not control it (maybe you might hack around this a bit). With session_set_save_handler() you can implement different locking strategies if you want to, for example guest accounts do not need locking because there is a negligible risk of session data corruption.

          I believe I understand the session problems and code perfectly, you do not need to explain anything to me. Really the 2.x session and 1.x legacy session code do not overlap much, the shared stuff is already in the session_stub class.

          Anyway you guys decide how to go forward, I do not mind if the session stuff gets reverted to 1.x state (aka legacy drivers only), do whatever you want. I do not have any energy left for persuading others that save_handler based session drivers do not allow some functionality to be implemented.

          Ciao

          Show
          Petr Škoda added a comment - - edited No. PHP save_handler sidesteps all our PHP code in session driver, it is really very different. Your memcached driver using save_handler fails badly for anything that needs to interfere with session timeouts for example. 1/ You do not control the session expiration done by save_handler apart from setting minimal timeout before the session gets discarded. SSO needs a lot more control over expiration of sessions, they must not just disappear. Really this can not be done with memcached or file save_handler. Functions registered via session_set_save_handler() can do anything you can imagine with the session timeouts (for example different timeouts for different account types). 2/ Yes, you can emulate the list of active sessions, but you need extra validation in session storage because thanks to 1/ you can not trust the contents of your emulated sessions table. 3/ When using save_handler the locking is all or nothing, again you do not control it (maybe you might hack around this a bit). With session_set_save_handler() you can implement different locking strategies if you want to, for example guest accounts do not need locking because there is a negligible risk of session data corruption. I believe I understand the session problems and code perfectly, you do not need to explain anything to me. Really the 2.x session and 1.x legacy session code do not overlap much, the shared stuff is already in the session_stub class. Anyway you guys decide how to go forward, I do not mind if the session stuff gets reverted to 1.x state (aka legacy drivers only), do whatever you want. I do not have any energy left for persuading others that save_handler based session drivers do not allow some functionality to be implemented. Ciao
          Hide
          Petr Škoda added a comment - - edited

          Here is my new attempt to reimplement moodle sessions from a fresh start: https://github.com/skodak/moodle/compare/wip_MDL-31501_m26_sessionreboot

          List of changes:

          • New OOP API using PHP namespace \core\session\.
          • All handlers now update the sessions table consistently.
          • Experimental DB session support in Oracle.
          • Full support for session file handler (filesystem locking required).
          • New option for alternative session directory.
          • Official memcached session handler support.
          • Workaround for memcached version with non-functional gc.
          • Improved security - forced session id regeneration.
          • Improved compatibility with recent PHP releases.
          • Fixed borked CSS during install in debug mode.
          • Switched to file based sessions in new installs.
          • DB session setting disappears if DB does not support sessions.
          • DB session setting disappears if session handler specified in config.php.
          • Fast purging of sessions used in request only.
          • No legacy distinction - file, database and memcached support the same functionality.
          • Other minor bugfixing and improvements.

          Limitations:

          • Session access time is now updated right after session start.
          • First request does not update userid in sessions table.
          • The timeouts may break badly if server hosting forces PHP.ini session settings.
          • The session GC is a lot slower, we do not rely on external session timeouts.
          • There cannot be any hooks triggered at the session write time.
          • File and memcached handlers do not support session lock acquire timeouts.
          • Some low level PHP session functions can not be used directly in Moodle code.

          TODOs:

          • Peer review.
          • Get agreement about the new direction.
          • Test, test, test, test...
          • Move deprecated functions to lib/deprecatedlib.php and update usage in the whole codebase.
          • More docs for admins and developers.
          Show
          Petr Škoda added a comment - - edited Here is my new attempt to reimplement moodle sessions from a fresh start: https://github.com/skodak/moodle/compare/wip_MDL-31501_m26_sessionreboot List of changes: New OOP API using PHP namespace \core\session\. All handlers now update the sessions table consistently. Experimental DB session support in Oracle. Full support for session file handler (filesystem locking required). New option for alternative session directory. Official memcached session handler support. Workaround for memcached version with non-functional gc. Improved security - forced session id regeneration. Improved compatibility with recent PHP releases. Fixed borked CSS during install in debug mode. Switched to file based sessions in new installs. DB session setting disappears if DB does not support sessions. DB session setting disappears if session handler specified in config.php. Fast purging of sessions used in request only. No legacy distinction - file, database and memcached support the same functionality. Other minor bugfixing and improvements. Limitations: Session access time is now updated right after session start. First request does not update userid in sessions table. The timeouts may break badly if server hosting forces PHP.ini session settings. The session GC is a lot slower, we do not rely on external session timeouts. There cannot be any hooks triggered at the session write time. File and memcached handlers do not support session lock acquire timeouts. Some low level PHP session functions can not be used directly in Moodle code. TODOs: Peer review. Get agreement about the new direction. Test, test, test, test... Move deprecated functions to lib/deprecatedlib.php and update usage in the whole codebase. More docs for admins and developers.
          Hide
          Martin Dougiamas added a comment -

          Well, thanks very much for all that work Petr.

          At a quick glance the structure looks good. I'm mostly concerned about the possibilities of regressions in core and 3rd party code, especially this close to a 2.6 code freeze, but I think it's worth kicking this hard to understand more about performance compared to older sessions (and the previous patches here).

          Would also appreciate reviews/thoughts/testing from anyone in the community who is interested in this.

          Show
          Martin Dougiamas added a comment - Well, thanks very much for all that work Petr. At a quick glance the structure looks good. I'm mostly concerned about the possibilities of regressions in core and 3rd party code, especially this close to a 2.6 code freeze, but I think it's worth kicking this hard to understand more about performance compared to older sessions (and the previous patches here). Would also appreciate reviews/thoughts/testing from anyone in the community who is interested in this.
          Hide
          Frédéric Massart added a comment -

          Assigning this to Petr.

          Show
          Frédéric Massart added a comment - Assigning this to Petr.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Petr,
          what about returning an error when $CFG->preventfilelocking has been configured for some reasons? Recently (MDL-41574) that setting has been revamped: time to deprecate it?

          Show
          Matteo Scaramuccia added a comment - Hi Petr, what about returning an error when $CFG->preventfilelocking has been configured for some reasons? Recently ( MDL-41574 ) that setting has been revamped: time to deprecate it?
          Hide
          Petr Škoda added a comment -

          Hello Matteo: yes, I was considering the $CFG->preventfilelocking too. I have tried to work around it by allowing admins to specify a directory for session outside of dataroot. I suppose that clusters should consider only memcached handler, all other installs can safely use local directory for sessions instead of NFS share without locking support.

          Show
          Petr Škoda added a comment - Hello Matteo: yes, I was considering the $CFG->preventfilelocking too. I have tried to work around it by allowing admins to specify a directory for session outside of dataroot. I suppose that clusters should consider only memcached handler, all other installs can safely use local directory for sessions instead of NFS share without locking support.
          Hide
          Frédéric Massart added a comment -

          Hi Petr,

          here are mostly some minor comments:

          DB handler

          1. No need to get all the fields for this, beside $DB->record_exists() should be enough.
          2. Can you mention the reason why we're not getting sessdata in the first query? It seems that in 99% of scenarios, we're querying the DB twice.
          3. Can you add some inline documentation that explains why you're trying to get the recordid in handler_write if you don't know it already. I assume this is because the record in the database could be created between handler_read and handler_write, but in most cases we would have the recordid.

          File handler

          1. Do we really have to make sure that the session directory is writable? It should be, especially if we're using $CFG->session_file_save_path. I agree that we need to check this, but it seems a bit excessive to do that on every single request. If this doesn't affect performances at all, then ignore me.
          2. Is it worth noting that save_path is not compatible with the format '5;/tmp'? http://au1.php.net/manual/en/session.configuration.php#ini.session.save-path

          Manager

          1. L43 What's the word na for?
          2. L52 Typo on form
          3. L108 Aren't we supposed to use html_writer everywhere now?
          4. L134 Doubled ;
          5. The prefix of the session cookie could be in a constant now.
          6. Why not calling initialise_empty_session() from initialise_user_session() when !$user? And what is this mnethostid = 1 here? (More inline doc?)
          7. In kill_all_sessions, shouldn't we delete everything before trying to contact the handlers? Just to make sure that an exception wouldn't prevent the delete to happen? Same for kill_session().
          8. In gc() it seems that you're always killing the session regardless of what the auth plugin returns.

          Memcached

          1. L56, it seems better to check if (empty($this->savepath)) as the config has already been checked.
          2. Not sure I like that kill_all_sessions queries the Database directly, not only there could be some keys left over on the Memcached server, but also there is a method to get all the keys, which could do the trick as well. If this needs the DB, than I would prefer have it stated somewhere that this can be done, or even is recommended, and that this is why the handler kill_all_sessions() occurs before the manager one.
          3. It's probably worth noting somewhere that this handler requires PECL Memcached 2.0.0.
          4. It makes me smile that you kept the very small piece of copyright and author from the original patch, I don't think it's supposed to be kept for only one method.

          General

          1. You're replacing SESSION_CUSTOM_CLASS with $CFG->session_handler_class. I guess this is OK because keeping the constant would lead to using a class that is outdated and extending unexisting classes. Though, is that OK to break things that way?
          2. Can we have some nice PHP Doc explaining exactly what to do in the handler constructor, and what to do in init? Are they expected to throw exceptions or to return False in case of problem, etc...

          I like the patch, and I think it's a nice clean up, as compared to the one I provided before it now supports killing a user session using a userid, and proper garbage collection. This is great but I am not sure this fulfills the expectations of the participants of this issue.

          • I noticed, no more session locking with other handlers than DB. That's a good thing.
          • Obviously using a separate session handler than DB saves the cost of transferring data from/to the DB.
          • There is still 1 read query on every single request.
          • The DB handler causes 3 read queries to acquire the session.

          I would suggest to introduce a logic that (if enabled) does not query the database to check if the session still exists, or if the timeout has expired. Of course this should not be enabled by default, but it could help improving performances for those who are willing to take some risks.

          • We could store the timestamp at which the session should expire to manually expire it if we don't access the DB value.
          • We could refetch the session data based on probability: rand(1, 5) % 5 == 0
          • And/or based on the last time we fetched it
          • And/or based on the number of requests since the last time we fetched it
          • And/or when something suspicious has happened, such as empty $_SESSION, which would mean that the handler has received the order to delete the session data. The admin have deleted a user for instance.

          That's it for me .

          Fred

          Show
          Frédéric Massart added a comment - Hi Petr, here are mostly some minor comments: DB handler No need to get all the fields for this, beside $DB->record_exists() should be enough. Can you mention the reason why we're not getting sessdata in the first query? It seems that in 99% of scenarios, we're querying the DB twice. Can you add some inline documentation that explains why you're trying to get the recordid in handler_write if you don't know it already. I assume this is because the record in the database could be created between handler_read and handler_write, but in most cases we would have the recordid. File handler Do we really have to make sure that the session directory is writable? It should be, especially if we're using $CFG->session_file_save_path. I agree that we need to check this, but it seems a bit excessive to do that on every single request. If this doesn't affect performances at all, then ignore me. Is it worth noting that save_path is not compatible with the format '5;/tmp'? http://au1.php.net/manual/en/session.configuration.php#ini.session.save-path Manager L43 What's the word na for? L52 Typo on form L108 Aren't we supposed to use html_writer everywhere now? L134 Doubled ; The prefix of the session cookie could be in a constant now. Why not calling initialise_empty_session() from initialise_user_session() when !$user? And what is this mnethostid = 1 here? (More inline doc?) In kill_all_sessions, shouldn't we delete everything before trying to contact the handlers? Just to make sure that an exception wouldn't prevent the delete to happen? Same for kill_session(). In gc() it seems that you're always killing the session regardless of what the auth plugin returns. Memcached L56, it seems better to check if (empty($this->savepath)) as the config has already been checked. Not sure I like that kill_all_sessions queries the Database directly, not only there could be some keys left over on the Memcached server, but also there is a method to get all the keys, which could do the trick as well. If this needs the DB, than I would prefer have it stated somewhere that this can be done, or even is recommended, and that this is why the handler kill_all_sessions() occurs before the manager one. It's probably worth noting somewhere that this handler requires PECL Memcached 2.0.0. It makes me smile that you kept the very small piece of copyright and author from the original patch, I don't think it's supposed to be kept for only one method. General You're replacing SESSION_CUSTOM_CLASS with $CFG->session_handler_class. I guess this is OK because keeping the constant would lead to using a class that is outdated and extending unexisting classes. Though, is that OK to break things that way? Can we have some nice PHP Doc explaining exactly what to do in the handler constructor, and what to do in init? Are they expected to throw exceptions or to return False in case of problem, etc... I like the patch, and I think it's a nice clean up, as compared to the one I provided before it now supports killing a user session using a userid, and proper garbage collection. This is great but I am not sure this fulfills the expectations of the participants of this issue. I noticed, no more session locking with other handlers than DB. That's a good thing. Obviously using a separate session handler than DB saves the cost of transferring data from/to the DB. There is still 1 read query on every single request. The DB handler causes 3 read queries to acquire the session. I would suggest to introduce a logic that (if enabled) does not query the database to check if the session still exists, or if the timeout has expired. Of course this should not be enabled by default, but it could help improving performances for those who are willing to take some risks. We could store the timestamp at which the session should expire to manually expire it if we don't access the DB value. We could refetch the session data based on probability: rand(1, 5) % 5 == 0 And/or based on the last time we fetched it And/or based on the number of requests since the last time we fetched it And/or when something suspicious has happened, such as empty $_SESSION, which would mean that the handler has received the order to delete the session data. The admin have deleted a user for instance. That's it for me . Fred
          Hide
          Petr Škoda added a comment -

          DB handler:

          1. I do not see any full record fetching, the first one was using "id,user", but I changed it to id only because the userid is not necessary any more
          2. the logic is get record id (necessary for locking), then get the lock, and only afterwards we can fetch the session data; fetching the sessdata first would even increase the probability of session overriding (we already had this bug before)
          3. added comment explaining that the session manager creates the record during the first access and replaced the sid fetching with update via sid

          File handler:

          1. read only sessions was a very common problems in 1.x, I think it is worth the extra operation; it is expensive to look for non-existent files and dirs because only existing file info is cached by PHP; +1 to measure it first on production servers before removing the test
          2. right, I have added test for multilevel save_path; maybe we could support it in the future

          Manager:

          1. na - it was a "name" but it was before I switch to full object instances in that property, fixed, thanks!
          2. form ==> from fixed
          3. html_writer was invented for developers that do not understand XSS, I do not think it is required, I just copy pasted that from existing code
          4. I do not see any benefit for constant there, it must not be used outside anyway
          5. done, empty session init is now there if !$user
          6. the handlers need to know the list of sessions in oder to delete them, they are not required to keep the list; if it fails there is a native GC fallback after a few days or the next time it will work
          7. yes, the primary task of gc is to purge records in sessions table, we give a chance the handlers to purge/release memory, we do not care if they do it or not; again the native GC after few days should kick in remove the leftovers

          Memcached:

          1. oh! using the $this->savepath now
          2. added note to kill_all_sessions() - Note: this can be significantly improved by fetching keys from memcached, but we need to make sure we are not deleting somebody elses sessions; in any case the native gc should kick in after few days (if it works)
          3. note about pecl extension version added, I suppose we will have a full docs page for memcached sessions because the config-dist.php is not enough imo.
          4. usually I rewrite those methods, but I kind of liked the implementation and could not come up with better one, so I kept it together with the credits

          I have crate a new branch, I will post about the general design later today, ciao and thanks again.

          Show
          Petr Škoda added a comment - DB handler: I do not see any full record fetching, the first one was using "id,user", but I changed it to id only because the userid is not necessary any more the logic is get record id (necessary for locking), then get the lock, and only afterwards we can fetch the session data; fetching the sessdata first would even increase the probability of session overriding (we already had this bug before) added comment explaining that the session manager creates the record during the first access and replaced the sid fetching with update via sid File handler: read only sessions was a very common problems in 1.x, I think it is worth the extra operation; it is expensive to look for non-existent files and dirs because only existing file info is cached by PHP; +1 to measure it first on production servers before removing the test right, I have added test for multilevel save_path; maybe we could support it in the future Manager: na - it was a "name" but it was before I switch to full object instances in that property, fixed, thanks! form ==> from fixed html_writer was invented for developers that do not understand XSS, I do not think it is required, I just copy pasted that from existing code I do not see any benefit for constant there, it must not be used outside anyway done, empty session init is now there if !$user the handlers need to know the list of sessions in oder to delete them, they are not required to keep the list; if it fails there is a native GC fallback after a few days or the next time it will work yes, the primary task of gc is to purge records in sessions table, we give a chance the handlers to purge/release memory, we do not care if they do it or not; again the native GC after few days should kick in remove the leftovers Memcached: oh! using the $this->savepath now added note to kill_all_sessions() - Note: this can be significantly improved by fetching keys from memcached, but we need to make sure we are not deleting somebody elses sessions; in any case the native gc should kick in after few days (if it works) note about pecl extension version added, I suppose we will have a full docs page for memcached sessions because the config-dist.php is not enough imo. usually I rewrite those methods, but I kind of liked the implementation and could not come up with better one, so I kept it together with the credits I have crate a new branch, I will post about the general design later today, ciao and thanks again.
          Hide
          Frédéric Massart added a comment -

          Petr, I randomly encountered this problem using the branch. Would that be related?

          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   1. {main}() /home/fred/www/repositories/sm/moodle/admin/index.php:0, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   2. moodle_database->__destruct() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:0, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   3. pgsql_native_moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:144, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   4. moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:208, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   5. core\\session\\manager::write_close() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:358, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   6. session_write_close() /home/fred/www/repositories/sm/moodle/lib/classes/session/manager.php:490, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   7. core\\session\\database->handler_write() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:0, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   8. moodle_database->set_field() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:225, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP   9. pgsql_native_moodle_database->set_field_select() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:1593, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP  10. pgsql_native_moodle_database->get_columns() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:1089, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP  11. cache::make() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:389, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP  12. cache_factory_disabled->create_cache_from_definition() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:171, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP  13. cache_factory_disabled->create_cache() /home/fred/www/repositories/sm/moodle/cache/disabledlib.php:217, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP  14. cache_factory->create_dummy_store() /home/fred/www/repositories/sm/moodle/cache/disabledlib.php:203, referer: http://fred.moodle.local/sm/
          [Fri Sep 13 10:57:22 2013] [notice] child pid 28339 exit signal Segmentation fault (11)
          
          Show
          Frédéric Massart added a comment - Petr, I randomly encountered this problem using the branch. Would that be related? [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP Stack trace:, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 1. {main}() /home/fred/www/repositories/sm/moodle/admin/index.php:0, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 2. moodle_database->__destruct() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:0, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 3. pgsql_native_moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:144, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 4. moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:208, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 5. core\\session\\manager::write_close() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:358, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 6. session_write_close() /home/fred/www/repositories/sm/moodle/lib/classes/session/manager.php:490, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 7. core\\session\\database->handler_write() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:0, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 8. moodle_database->set_field() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:225, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 9. pgsql_native_moodle_database->set_field_select() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:1593, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 10. pgsql_native_moodle_database->get_columns() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:1089, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 11. cache::make() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:389, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 12. cache_factory_disabled->create_cache_from_definition() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:171, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 13. cache_factory_disabled->create_cache() /home/fred/www/repositories/sm/moodle/cache/disabledlib.php:217, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:21 2013] [error] [client 192.168.100.54] PHP 14. cache_factory->create_dummy_store() /home/fred/www/repositories/sm/moodle/cache/disabledlib.php:203, referer: http://fred.moodle.local/sm/ [Fri Sep 13 10:57:22 2013] [notice] child pid 28339 exit signal Segmentation fault (11)
          Hide
          Martin Dougiamas added a comment -

          What would really help everyone understand sessions better (and this issue) are some flow diagrams like these http://docs.moodle.org/dev/Automatic_updates_deployment

          Can you, Petr, please either produce such a diagram or at least write some pseudo code here that explains, for all implemented session types (database, file and memcached), exactly what the flow is? I'm particularly interested to know exactly when Moodle databases are being accessed in all cases, ultimately so I can understand how much of a performance increase there is going to be with using memcached.

          Show
          Martin Dougiamas added a comment - What would really help everyone understand sessions better (and this issue) are some flow diagrams like these http://docs.moodle.org/dev/Automatic_updates_deployment Can you, Petr, please either produce such a diagram or at least write some pseudo code here that explains, for all implemented session types (database, file and memcached), exactly what the flow is? I'm particularly interested to know exactly when Moodle databases are being accessed in all cases, ultimately so I can understand how much of a performance increase there is going to be with using memcached.
          Hide
          Petr Škoda added a comment -

          The code flow is the same as with ordinary PHP sessions, the only exception is that after starting session the ''sessions'' database table is verified and updated if necessary. The standard garbage collection is still in place with a very long lifetime (a few days). The session timeouts are verified after the session is started - this is the trick that allows us to control any timeout actions.

          The code flow is:
          1/ session is started from lib/setup.php - the actual code is in \core\session\manager
          2/ right after session starts we read the sessions table record using session id identifier, verify the data and update it if necessary (userid changed, timemodified was not updated in the last 20 seconds, etc.), sessions timeouts are verified in this step
          3/ PHP handles the writing of session data to storage

          The new session handlers (file, memcached and database) set up standard session save handlers or PHP session callbacks, they do not do anything special during normal page access. The extra API allows the handlers to purge the session data during cron bulk session timeout processing.

          The number of DB queries is exactly the same for file and memcached handlers - 1 sessions table read per page and 1 optional update from time to time. In case of the database driver it needs to also acquire lock, fetch session data, update session data and release the lock. The database handler emulates standard session save handler functionality, there is no special code, it is consistent with the other two drivers. It was significantly simplified compared to 2.5 codebase.

          The perf hacks above proposed by Fred above are in my opinion dangerous, one read + less than one write per page are worth it because the result is more reliable in my opinion. In any case it is just an internal detail in session manager implementation and the session handlers would not be affected, this means we could optimise this in the future without breaking BC.

          Show
          Petr Škoda added a comment - The code flow is the same as with ordinary PHP sessions, the only exception is that after starting session the ''sessions'' database table is verified and updated if necessary. The standard garbage collection is still in place with a very long lifetime (a few days). The session timeouts are verified after the session is started - this is the trick that allows us to control any timeout actions. The code flow is: 1/ session is started from lib/setup.php - the actual code is in \core\session\manager 2/ right after session starts we read the sessions table record using session id identifier, verify the data and update it if necessary (userid changed, timemodified was not updated in the last 20 seconds, etc.), sessions timeouts are verified in this step 3/ PHP handles the writing of session data to storage The new session handlers (file, memcached and database) set up standard session save handlers or PHP session callbacks, they do not do anything special during normal page access. The extra API allows the handlers to purge the session data during cron bulk session timeout processing. The number of DB queries is exactly the same for file and memcached handlers - 1 sessions table read per page and 1 optional update from time to time. In case of the database driver it needs to also acquire lock, fetch session data, update session data and release the lock. The database handler emulates standard session save handler functionality, there is no special code, it is consistent with the other two drivers. It was significantly simplified compared to 2.5 codebase. The perf hacks above proposed by Fred above are in my opinion dangerous, one read + less than one write per page are worth it because the result is more reliable in my opinion. In any case it is just an internal detail in session manager implementation and the session handlers would not be affected, this means we could optimise this in the future without breaking BC.
          Hide
          Petr Škoda added a comment -

          Submitting for one more peer review, it is now ready for integration in my opinion...

          Show
          Petr Škoda added a comment - Submitting for one more peer review, it is now ready for integration in my opinion...
          Hide
          Petr Škoda added a comment -

          Frédéric Massart: it might be related to circular references in database handlers, it does not happen in my PHP 5.5.x install, what PHP version and build do you have? If we manage to reproduce it we could try to come up with some reference breaking hacks in shutdown handlers. Anyway I hope that most sites will use memcached or file based handlers now.

          Show
          Petr Škoda added a comment - Frédéric Massart : it might be related to circular references in database handlers, it does not happen in my PHP 5.5.x install, what PHP version and build do you have? If we manage to reproduce it we could try to come up with some reference breaking hacks in shutdown handlers. Anyway I hope that most sites will use memcached or file based handlers now.
          Hide
          Frédéric Massart added a comment - - edited

          Hi Petr,

          Here is my second review:

          1. In the server settings you are using $CFG->session_handler instead of $CFG->session_handler_class
          2. Have we agreed on not repeating the PHP Docs when they are already defined in the parent class? I am not in favor of this, but I just want to make sure there is an agreement. (This can be really annoying when using a more simpler code editor)
          3. Not sure if we need to perform the "Multilevel session directories" test on every single request. Some config-dist documentation might be enough. This is a fairly quick operation, but I just wonder if it is required.
          4. I am asking this question again: Is it right to break the BC for existing custom handlers the hard way? No more session_stub, must use autoloading, ...
          5. Still double semi-colon on L134 of manager.
          6. I would still prefer a constant rather than raw text for the session prefix, I know it's not re-used outside, but it prevents typo and is cleaner IMO.
          7. Missing PHP Docs from add_session_record().
          8. In manager::login_user() you delete the records from the session, shouldn't you call the handlers::kill_session($sid) too? The same happens in terminate_current().
          9. In Memcached, should we force a prefix of the prefix? Like site identifier? In order to prevent shared session data for those who misconfigure their config.php? If not, then probably adding some more documentation explaining the risks when 2 Moodles inherit the same ini_get() prefix.
          10. About the PECL version, you could also use `version_compare($memcachedversion, '2.0') < 0`.

          About the exception, I don't think this has anything to do with my PHP version:

          PHP 5.3.10-1ubuntu3.8 with Suhosin-Patch (cli) (built: Sep  4 2013 20:00:51) 
          Copyright (c) 1997-2012 The PHP Group
          Zend Engine v2.3.0, Copyright (c) 1998-2012 Zend Technologies
              with Zend OPcache v7.0.2, Copyright (c) 1999-2013, by Zend Technologies
              with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans
          

          The problem arises because there are uses of $CFG and stuff during the __deconstruct() of everything. I could randomely reproduce this by:

          • Removing any debug definition in config.php
          • Manually purging all the cache
          • Deleting all the records in mdl_sessions
          • Logging in (not sure about that)
          • Accessing admin/index.php

          This seems to have a positive effect:

          diff --git a/cache/classes/factory.php b/cache/classes/factory.php
          index 476ec0b..620dd31 100644
          --- a/cache/classes/factory.php
          +++ b/cache/classes/factory.php
          @@ -426,8 +426,8 @@ class cache_factory {
                * @return cachestore_dummy
                */
               protected function create_dummy_store(cache_definition $definition) {
          -        global $CFG;
          -        require_once($CFG->dirroot.'/cache/classes/dummystore.php');
          +        // global $CFG;
          +        require_once(__DIR__.'/dummystore.php');
                   $store = new cachestore_dummy();
                   $store->initialise($definition);
                   return $store;
          

          That also made me discover another issue that should be fixed:

          diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php
          index 150b854..53aeb0e 100644
          --- a/lib/dml/moodle_database.php
          +++ b/lib/dml/moodle_database.php
          @@ -354,9 +354,7 @@ abstract class moodle_database {
                   }
                   // Always terminate sessions here to make it consistent,
                   // this is needed because we need to save session to db before closing it.
          -        if (function_exists('session_get_instance')) {
          -            \core\session\manager::write_close();
          -        }
          +        \core\session\manager::write_close();
                   $this->used_for_db_sessions = false;
           
                   if ($this->temptables) {
          

          Another notice that was displayed:

          Notice: Trying to get property of non-object in /home/fred/www/repositories/sm/moodle/lib/setuplib.php on line 1255
          
          Call Stack:
              0.4228   49325064   1. moodle_database->__destruct() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:0
              0.4228   49325064   2. pgsql_native_moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:144
              0.4228   49325064   3. moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:208
              0.4228   49325064   4. core\session\manager::write_close() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:358
              0.4228   49325144   5. session_write_close() /home/fred/www/repositories/sm/moodle/lib/classes/session/manager.php:487
              0.4228   49328560   6. core\session\database->handler_write() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:0
              0.4229   49331464   7. moodle_database->set_field() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:218
              0.4229   49332312   8. pgsql_native_moodle_database->set_field_select() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:1593
              0.4229   49333200   9. pgsql_native_moodle_database->get_columns() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:1089
              0.4244   49370304  10. cache_application->set() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:573
              0.4244   49370304  11. cache->set() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:1293
              0.4246   49370544  12. cachestore_file->set() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:505
              0.4246   49370800  13. cachestore_file->file_path_for_key() /home/fred/www/repositories/sm/moodle/cache/stores/file/lib.php:435
              0.4246   49371264  14. make_writable_directory() /home/fred/www/repositories/sm/moodle/cache/stores/file/lib.php:323
          

          I think this patch is close to be ready for integration, but first there should be some more research on the usage of cache and other things at the time where some references are lost.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited Hi Petr, Here is my second review: In the server settings you are using $CFG->session_handler instead of $CFG->session_handler_class Have we agreed on not repeating the PHP Docs when they are already defined in the parent class? I am not in favor of this, but I just want to make sure there is an agreement. (This can be really annoying when using a more simpler code editor) Not sure if we need to perform the "Multilevel session directories" test on every single request. Some config-dist documentation might be enough. This is a fairly quick operation, but I just wonder if it is required. I am asking this question again: Is it right to break the BC for existing custom handlers the hard way? No more session_stub, must use autoloading, ... Still double semi-colon on L134 of manager. I would still prefer a constant rather than raw text for the session prefix, I know it's not re-used outside, but it prevents typo and is cleaner IMO. Missing PHP Docs from add_session_record(). In manager::login_user() you delete the records from the session, shouldn't you call the handlers::kill_session($sid) too? The same happens in terminate_current(). In Memcached, should we force a prefix of the prefix? Like site identifier? In order to prevent shared session data for those who misconfigure their config.php? If not, then probably adding some more documentation explaining the risks when 2 Moodles inherit the same ini_get() prefix. About the PECL version, you could also use `version_compare($memcachedversion, '2.0') < 0`. About the exception, I don't think this has anything to do with my PHP version: PHP 5.3.10-1ubuntu3.8 with Suhosin-Patch (cli) (built: Sep 4 2013 20:00:51) Copyright (c) 1997-2012 The PHP Group Zend Engine v2.3.0, Copyright (c) 1998-2012 Zend Technologies with Zend OPcache v7.0.2, Copyright (c) 1999-2013, by Zend Technologies with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans The problem arises because there are uses of $CFG and stuff during the __deconstruct() of everything. I could randomely reproduce this by: Removing any debug definition in config.php Manually purging all the cache Deleting all the records in mdl_sessions Logging in (not sure about that) Accessing admin/index.php This seems to have a positive effect: diff --git a/cache/classes/factory.php b/cache/classes/factory.php index 476ec0b..620dd31 100644 --- a/cache/classes/factory.php +++ b/cache/classes/factory.php @@ -426,8 +426,8 @@ class cache_factory { * @return cachestore_dummy */ protected function create_dummy_store(cache_definition $definition) { - global $CFG; - require_once($CFG->dirroot.'/cache/classes/dummystore.php'); + // global $CFG; + require_once(__DIR__.'/dummystore.php'); $store = new cachestore_dummy(); $store->initialise($definition); return $store; That also made me discover another issue that should be fixed: diff --git a/lib/dml/moodle_database.php b/lib/dml/moodle_database.php index 150b854..53aeb0e 100644 --- a/lib/dml/moodle_database.php +++ b/lib/dml/moodle_database.php @@ -354,9 +354,7 @@ abstract class moodle_database { } // Always terminate sessions here to make it consistent, // this is needed because we need to save session to db before closing it. - if (function_exists('session_get_instance')) { - \core\session\manager::write_close(); - } + \core\session\manager::write_close(); $this->used_for_db_sessions = false; if ($this->temptables) { Another notice that was displayed: Notice: Trying to get property of non-object in /home/fred/www/repositories/sm/moodle/lib/setuplib.php on line 1255 Call Stack: 0.4228 49325064 1. moodle_database->__destruct() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:0 0.4228 49325064 2. pgsql_native_moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:144 0.4228 49325064 3. moodle_database->dispose() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:208 0.4228 49325064 4. core\session\manager::write_close() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:358 0.4228 49325144 5. session_write_close() /home/fred/www/repositories/sm/moodle/lib/classes/session/manager.php:487 0.4228 49328560 6. core\session\database->handler_write() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:0 0.4229 49331464 7. moodle_database->set_field() /home/fred/www/repositories/sm/moodle/lib/classes/session/database.php:218 0.4229 49332312 8. pgsql_native_moodle_database->set_field_select() /home/fred/www/repositories/sm/moodle/lib/dml/moodle_database.php:1593 0.4229 49333200 9. pgsql_native_moodle_database->get_columns() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:1089 0.4244 49370304 10. cache_application->set() /home/fred/www/repositories/sm/moodle/lib/dml/pgsql_native_moodle_database.php:573 0.4244 49370304 11. cache->set() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:1293 0.4246 49370544 12. cachestore_file->set() /home/fred/www/repositories/sm/moodle/cache/classes/loaders.php:505 0.4246 49370800 13. cachestore_file->file_path_for_key() /home/fred/www/repositories/sm/moodle/cache/stores/file/lib.php:435 0.4246 49371264 14. make_writable_directory() /home/fred/www/repositories/sm/moodle/cache/stores/file/lib.php:323 I think this patch is close to be ready for integration, but first there should be some more research on the usage of cache and other things at the time where some references are lost. Cheers, Fred
          Hide
          Martin Dougiamas added a comment -

          OK, all sounding pretty good to me. Compared to 2.5 a cluster system can now significantly reduce the database traffic which I think is a big win for 2.6.

          Really hoping David M can generate some good performance comparisons here soon.

          Show
          Martin Dougiamas added a comment - OK, all sounding pretty good to me. Compared to 2.5 a cluster system can now significantly reduce the database traffic which I think is a big win for 2.6. Really hoping David M can generate some good performance comparisons here soon.
          Hide
          Petr Škoda added a comment - - edited

          1/ thanks - settings are fixed
          2/ I have copied phpdocs everywhere for now
          3/ I would prefer to actually add multilevel session support. If we do not check the path value it would create random dirs in moodle which is imo bad. + to improve it later if necessary
          4/ I was thinking about the BC - I decided to use new setting for session handler class and remove all previous code because I think there is no need for custom handlers any more - I did not find any other suitable backend that would support proper locking.
          5/ thanks - ;; removed
          6/ I have used a variable for the session name, after the cookie init the rest of the code should use standard session_name()
          7/ added missing phpdoc
          8/ manager__login_user() is using session_regenerate_id(true), the true parameter does the purging of old session data; terminate_current() does extra tricks to work around already closed session or other problems
          9/ my code does not require unique prefixes in memcached server because it always uses the data from sessions table and the sid collision is highly unlikely; in any case I am sure there is a lot of room for improvements in the memcached driver, let's work on that later if necessary.
          10/ memcached version test added - thanks for telling how to do it easily

          It seems we have also resolved the problems with segfaults in shutdown handler, the solution was to hold reference to session meta cache and register own shutdown function in database session handler.

          Thanks a lot! Submitting for integration...

          Show
          Petr Škoda added a comment - - edited 1/ thanks - settings are fixed 2/ I have copied phpdocs everywhere for now 3/ I would prefer to actually add multilevel session support. If we do not check the path value it would create random dirs in moodle which is imo bad. + to improve it later if necessary 4/ I was thinking about the BC - I decided to use new setting for session handler class and remove all previous code because I think there is no need for custom handlers any more - I did not find any other suitable backend that would support proper locking. 5/ thanks - ;; removed 6/ I have used a variable for the session name, after the cookie init the rest of the code should use standard session_name() 7/ added missing phpdoc 8/ manager__login_user() is using session_regenerate_id(true), the true parameter does the purging of old session data; terminate_current() does extra tricks to work around already closed session or other problems 9/ my code does not require unique prefixes in memcached server because it always uses the data from sessions table and the sid collision is highly unlikely; in any case I am sure there is a lot of room for improvements in the memcached driver, let's work on that later if necessary. 10/ memcached version test added - thanks for telling how to do it easily It seems we have also resolved the problems with segfaults in shutdown handler, the solution was to hold reference to session meta cache and register own shutdown function in database session handler. Thanks a lot! Submitting for integration...
          Hide
          Kris Stokking added a comment -

          Mark Nielsen and I chatted with Petr about the new version of sessions last week. As Petr has explained, this version handles session timeouts for all sessions by reading and periodically writing useful information to the sessions table. This is similar to what we had been previously proposing, except that it happens on every request instead of only on login so that it can fully control timeouts.

          There is a slight performance hit for doing so but it should be fairly negligible because the DB calls (on each page load) take advantage of unique indexes and they aren't sending a ton of data. Also, the write frequency ($CFG->session_update_timemodified_frequency) can be tuned to cut down DB traffic as needed. Since most implementations will only rely on writes to handle session expiration (I think), it shouldn't be detrimental to put that at a very high number such as 300 (5 minutes) for a 2 hour session. Honestly, who's going to know the difference. This feature was very important to me to keep session handling as performant as possible. +1 on the design changes, very nice job Petr! I'm looking forward to seeing the performance results.

          Final thoughts:

          • Important: PECL extension must be >= 2.0.1 if we are aiming to avoid the bug with memcached sessions never expiring. See my comment for more info.
          • We probably want to change the title and description of this ticket since it has morphed into a rewrite of sessions.
          Show
          Kris Stokking added a comment - Mark Nielsen and I chatted with Petr about the new version of sessions last week. As Petr has explained, this version handles session timeouts for all sessions by reading and periodically writing useful information to the sessions table. This is similar to what we had been previously proposing, except that it happens on every request instead of only on login so that it can fully control timeouts. There is a slight performance hit for doing so but it should be fairly negligible because the DB calls (on each page load) take advantage of unique indexes and they aren't sending a ton of data. Also, the write frequency ($CFG->session_update_timemodified_frequency) can be tuned to cut down DB traffic as needed. Since most implementations will only rely on writes to handle session expiration (I think), it shouldn't be detrimental to put that at a very high number such as 300 (5 minutes) for a 2 hour session. Honestly, who's going to know the difference. This feature was very important to me to keep session handling as performant as possible. +1 on the design changes, very nice job Petr! I'm looking forward to seeing the performance results. Final thoughts: Important: PECL extension must be >= 2.0.1 if we are aiming to avoid the bug with memcached sessions never expiring. See my comment for more info. We probably want to change the title and description of this ticket since it has morphed into a rewrite of sessions.
          Hide
          Petr Škoda added a comment -

          Hi, thanks for the info. memcached 2.0.0 should be ok because we track all sessions in database and GC them from PHP code, the lack of native GC should not cause serious problems imho.

          Show
          Petr Škoda added a comment - Hi, thanks for the info. memcached 2.0.0 should be ok because we track all sessions in database and GC them from PHP code, the lack of native GC should not cause serious problems imho.
          Hide
          David Monllaó added a comment -

          Matthew Spurrier is setting up a test environment

          Show
          David Monllaó added a comment - Matthew Spurrier is setting up a test environment
          Hide
          Sam Hemelryk added a comment -

          After looking at this for an extended period of time there was nothing I spotted that needed to be addressed.
          Excellent job guys, lets see where it takes us.

          This has been integrated now.

          Show
          Sam Hemelryk added a comment - After looking at this for an extended period of time there was nothing I spotted that needed to be addressed. Excellent job guys, lets see where it takes us. This has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Grrr there was a bug in there. Installation (both web and cli) broke after this change as the install scripts didn't specify the autoloader.
          To replicate:

          • delete you config.php file
          • attempt an install.

          I've integrated a commit now that copies the autoloader call to both install scripts `git show d99847a..98d696b`

          Petr can you please review my commit and confirm it is the correct fix, if not please create the correct fix and let me know when its ready.
          Could you also please update the test instructions to include testing a perfectly clean install.

          Show
          Sam Hemelryk added a comment - Grrr there was a bug in there. Installation (both web and cli) broke after this change as the install scripts didn't specify the autoloader. To replicate: delete you config.php file attempt an install. I've integrated a commit now that copies the autoloader call to both install scripts `git show d99847a..98d696b` Petr can you please review my commit and confirm it is the correct fix, if not please create the correct fix and let me know when its ready. Could you also please update the test instructions to include testing a perfectly clean install.
          Hide
          Petr Škoda added a comment -

          ooops, sorry, I was considering adding the classloader there myself, +1 for your change, thanks a lot

          Show
          Petr Škoda added a comment - ooops, sorry, I was considering adding the classloader there myself, +1 for your change, thanks a lot
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm… something continues wrong here. The CI servers have stopped working and it seems this is the last stuff to land.

          They just fail trying to use the standard cli installer:

          php admin/cli/install.php --non-interactive --allow-unstable --agree-license --wwwroot=http://localhost --dataroot=/tmp/ci_dataroot_2424_0 --dbtype=mysqli --dbhost=localhost --dbname=ci_installed_2424_0 --dbuser=xxxxx --dbpass=xxxxx --prefix=cii_ --fullname=ci_installed_2424_0 --shortname=ci_installed_2424_0 --adminuser=xxxxx --adminpass=xxxxx
          
          
          Notice: Trying to get property of non-object in /Users/stronk7/git_moodle/integration/lib/upgradelib.php on line 1232
          
          Call Stack:
              0.0016     418752   1. {main}() /admin/cli/install.php:0
              1.3154   28031352   2. install_cli_database() /admin/cli/install.php:742
              1.3191   28518536   3. install_core() /lib/installlib.php:473
              1.3244   28695976   4. upgrade_handle_exception() /lib/upgradelib.php:1508
              1.3249   28701072   5. upgrade_log() /lib/upgradelib.php:1139
          
          Default exception handler: Coding error detected, it must be fixed by a programmer: PHP catchable fatal error Debug: Argument 1 passed to check_user_preferences_loaded() must be an instance of stdClass, null given, called in [dirroot]/lib/moodlelib.php on line 1995 and defined
          Error code: codingerror
          * line 393 of /lib/setuplib.php: coding_exception thrown
          * line 1748 of /lib/moodlelib.php: call to default_error_handler()
          * line 1995 of /lib/moodlelib.php: call to check_user_preferences_loaded()
          * line 261 of /lib/classes/useragent.php: call to get_user_preferences()
          * line 662 of /lib/pagelib.php: call to core_useragent::get_user_device_type()
          * line 734 of /lib/pagelib.php: call to moodle_page->magic_get_devicetypeinuse()
          * line 1552 of /lib/pagelib.php: call to moodle_page->__get()
          * line 1466 of /lib/pagelib.php: call to moodle_page->resolve_theme()
          * line 1538 of /lib/setuplib.php: call to moodle_page->initialise_theme_and_output()
          * line 1354 of /lib/upgradelib.php: call to bootstrap_renderer->__call()
          * line 1354 of /lib/upgradelib.php: call to bootstrap_renderer->heading()
          * line 1481 of /lib/upgradelib.php: call to print_upgrade_part_start()
          * line 473 of /lib/installlib.php: call to install_core()
          * line 742 of /admin/cli/install.php: call to install_cli_database()
          
          !!! Coding error detected, it must be fixed by a programmer: PHP catchable fatal error !!!
          
          Show
          Eloy Lafuente (stronk7) added a comment - Uhm… something continues wrong here. The CI servers have stopped working and it seems this is the last stuff to land. They just fail trying to use the standard cli installer: php admin/cli/install.php --non-interactive --allow-unstable --agree-license --wwwroot=http: //localhost --dataroot=/tmp/ci_dataroot_2424_0 --dbtype=mysqli --dbhost=localhost --dbname=ci_installed_2424_0 --dbuser=xxxxx --dbpass=xxxxx --prefix=cii_ --fullname=ci_installed_2424_0 --shortname=ci_installed_2424_0 --adminuser=xxxxx --adminpass=xxxxx Notice: Trying to get property of non-object in /Users/stronk7/git_moodle/integration/lib/upgradelib.php on line 1232 Call Stack: 0.0016 418752 1. {main}() /admin/cli/install.php:0 1.3154 28031352 2. install_cli_database() /admin/cli/install.php:742 1.3191 28518536 3. install_core() /lib/installlib.php:473 1.3244 28695976 4. upgrade_handle_exception() /lib/upgradelib.php:1508 1.3249 28701072 5. upgrade_log() /lib/upgradelib.php:1139 Default exception handler: Coding error detected, it must be fixed by a programmer: PHP catchable fatal error Debug: Argument 1 passed to check_user_preferences_loaded() must be an instance of stdClass, null given, called in [dirroot]/lib/moodlelib.php on line 1995 and defined Error code: codingerror * line 393 of /lib/setuplib.php: coding_exception thrown * line 1748 of /lib/moodlelib.php: call to default_error_handler() * line 1995 of /lib/moodlelib.php: call to check_user_preferences_loaded() * line 261 of /lib/classes/useragent.php: call to get_user_preferences() * line 662 of /lib/pagelib.php: call to core_useragent::get_user_device_type() * line 734 of /lib/pagelib.php: call to moodle_page->magic_get_devicetypeinuse() * line 1552 of /lib/pagelib.php: call to moodle_page->__get() * line 1466 of /lib/pagelib.php: call to moodle_page->resolve_theme() * line 1538 of /lib/setuplib.php: call to moodle_page->initialise_theme_and_output() * line 1354 of /lib/upgradelib.php: call to bootstrap_renderer->__call() * line 1354 of /lib/upgradelib.php: call to bootstrap_renderer->heading() * line 1481 of /lib/upgradelib.php: call to print_upgrade_part_start() * line 473 of /lib/installlib.php: call to install_core() * line 742 of /admin/cli/install.php: call to install_cli_database() !!! Coding error detected, it must be fixed by a programmer: PHP catchable fatal error !!!
          Hide
          Petr Škoda added a comment -

          verifying...

          Show
          Petr Škoda added a comment - verifying...
          Hide
          Petr Škoda added a comment -

          Eloy: could you please try this https://github.com/skodak/moodle/commits/int_install ?

          Show
          Petr Škoda added a comment - Eloy: could you please try this https://github.com/skodak/moodle/commits/int_install ?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Seems to be working here… so applying.. thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Seems to be working here… so applying.. thanks!
          Hide
          David Monllaó added a comment -

          All behat tests using the courses data generator are failing because $USER is not properly set, attaching a fix to set $USER when the behat CLI command is running (only CLI, not when the behat site is accessed through the browser) Patch on top of current master-integration.

          git pull git://github.com/dmonllao/moodle.git MDL-31501-behat-fix

          Show
          David Monllaó added a comment - All behat tests using the courses data generator are failing because $USER is not properly set, attaching a fix to set $USER when the behat CLI command is running (only CLI, not when the behat site is accessed through the browser) Patch on top of current master-integration. git pull git://github.com/dmonllao/moodle.git MDL-31501 -behat-fix
          Hide
          Petr Škoda added a comment -

          Hi David, I do not think your commit is a proper fix, I believe Behat code needs to change the way it modifies USER and SESSION instead.

          Show
          Petr Škoda added a comment - Hi David, I do not think your commit is a proper fix, I believe Behat code needs to change the way it modifies USER and SESSION instead.
          Hide
          Petr Škoda added a comment -

          we are testing a fix for the behat regression, I will post a branch here soon, sorry

          Show
          Petr Škoda added a comment - we are testing a fix for the behat regression, I will post a branch here soon, sorry
          Hide
          Petr Škoda added a comment -

          to integrators: please merge https://github.com/skodak/moodle/commits/int_behat
          thanks

          Show
          Petr Škoda added a comment - to integrators: please merge https://github.com/skodak/moodle/commits/int_behat thanks
          Hide
          Sam Hemelryk added a comment -

          Thanks Petr that fix has been pulled as well

          Show
          Sam Hemelryk added a comment - Thanks Petr that fix has been pulled as well
          Hide
          David Monllaó added a comment -

          I'm running performance tests with/without the patch using db sessions and the difference in DB writes is specially significant with MDL-31501 integrated. The main difference I see is that, being logged, mdl_session->timemodified now is updated in initialise_user_session() rather than together with the session info when all the page processes are done, in handler_write(). I haven't followed the issue progress but see a few comments about the timemodified frequency so I guess that you are aware of it but just in case, with $CFG->session_update_timemodified_frequency to 20 seconds (default value) and using db sessions is likely that there will be 2 mdl_session writes for each page view in real situations rather than just one like right now.

          Show
          David Monllaó added a comment - I'm running performance tests with/without the patch using db sessions and the difference in DB writes is specially significant with MDL-31501 integrated. The main difference I see is that, being logged, mdl_session->timemodified now is updated in initialise_user_session() rather than together with the session info when all the page processes are done, in handler_write(). I haven't followed the issue progress but see a few comments about the timemodified frequency so I guess that you are aware of it but just in case, with $CFG->session_update_timemodified_frequency to 20 seconds (default value) and using db sessions is likely that there will be 2 mdl_session writes for each page view in real situations rather than just one like right now.
          Hide
          Petr Škoda added a comment -

          If you care about performance simply do not use the database sessions, the default is now file based sessions in new installs.

          Show
          Petr Škoda added a comment - If you care about performance simply do not use the database sessions, the default is now file based sessions in new installs.
          Hide
          Andrew Davis added a comment -

          update on the testing. I'm still testing this.

          I have the unit tests running using database sessions. Thats been running all morning and is currently at 1525 / 2215. Then I'll do the manual testing.

          I finally got 2.2 to install (our download site gave me a corrupt archive twice, 2.2 installation in postgres fails with a DDL exception). I've set that site to to use file sessions. I'll upgrade that then use it run unit tests and do the manual testing.

          The only one I havent touched is memcached.

          Show
          Andrew Davis added a comment - update on the testing. I'm still testing this. I have the unit tests running using database sessions. Thats been running all morning and is currently at 1525 / 2215. Then I'll do the manual testing. I finally got 2.2 to install (our download site gave me a corrupt archive twice, 2.2 installation in postgres fails with a DDL exception). I've set that site to to use file sessions. I'll upgrade that then use it run unit tests and do the manual testing. The only one I havent touched is memcached.
          Hide
          Andrew Davis added a comment -

          The unit tests on the site using database sessions reported some errors but nothing that looks likely to be caused by this issue. "SQL operator "=" is expected to be case sensitive", "Problem detected in environment (php_extension:xmlrpc)"

          I've successfully upgraded from 2.2 to 2.6 in the site using file session storage. The unit tests are running now. The testing continues...

          Show
          Andrew Davis added a comment - The unit tests on the site using database sessions reported some errors but nothing that looks likely to be caused by this issue. "SQL operator "=" is expected to be case sensitive", "Problem detected in environment (php_extension:xmlrpc)" I've successfully upgraded from 2.2 to 2.6 in the site using file session storage. The unit tests are running now. The testing continues...
          Hide
          Andrew Davis added a comment - - edited

          The unit tests completed fine for file sessions storage.

          I have now completed all the manual testing for database sessions. It all appears to be working fine.

          The manual testing of the file session storage site is also complete.

          I've also installed a new site and verified that file based session are used by default.

          Just waiting on Michael who is testing memcached and I think this can be passed.

          Show
          Andrew Davis added a comment - - edited The unit tests completed fine for file sessions storage. I have now completed all the manual testing for database sessions. It all appears to be working fine. The manual testing of the file session storage site is also complete. I've also installed a new site and verified that file based session are used by default. Just waiting on Michael who is testing memcached and I think this can be passed.
          Hide
          Michael de Raadt added a comment -

          I tested this with memcached. It took me a long time to get this installed. I think it would be easier of you had the latest Ubuntu OS and PHP 5.4, but installing it manually was a trial. Undoubtedly others will want to know how to do this so the docs should probably send them in the right direction.

          Once installed I was able to see the sessions changing. The PHPUnit tests passed. Users logged into multiple browsers were controlled.

          I did get one warning...

          Warning: session_start(): Unable to clear session lock record in /home/michael/web/htdocs/25_integration_trash/lib/classes/session/manager.php on line 77 
          

          I tried to get the locking working using the following script...

          <?php
          include('config.php');
          echo 'starting<br />';
          flush();
          sleep(30);
          echo 'stopping';
          

          ...but while that was running, I was still able to use the same site for other things.

          Show
          Michael de Raadt added a comment - I tested this with memcached. It took me a long time to get this installed. I think it would be easier of you had the latest Ubuntu OS and PHP 5.4, but installing it manually was a trial. Undoubtedly others will want to know how to do this so the docs should probably send them in the right direction. Once installed I was able to see the sessions changing. The PHPUnit tests passed. Users logged into multiple browsers were controlled. I did get one warning... Warning: session_start(): Unable to clear session lock record in /home/michael/web/htdocs/25_integration_trash/lib/classes/session/manager.php on line 77 I tried to get the locking working using the following script... <?php include('config.php'); echo 'starting<br />'; flush(); sleep(30); echo 'stopping'; ...but while that was running, I was still able to use the same site for other things.
          Hide
          Petr Škoda added a comment - - edited

          If the session locking does not work it is a problem in PHP memcached extension or memcached server, our PHP code cannot affect that.

          Show
          Petr Škoda added a comment - - edited If the session locking does not work it is a problem in PHP memcached extension or memcached server, our PHP code cannot affect that.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          I've tried here memcached sessions with a sleep(230) and can confirm that the session is getting locked and cannot do anything in the site with the same browser (aka, same session).

          Both same user and different user in another browser (aka, different sessions) worked ok.

          LOL, waiting the 230 secs to expire…

          Perfect, now can continue using the site.

          So I'd say memcached locking is working… if that was the only problem… I'd propose to pass this. Any objection?

          Side comment: Petr, it would be great to have some way to know, from the web UI, which session handler is being used (footer?). Having that only available via config.php and without any feedback… it's a bit like Saint Thomas.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited I've tried here memcached sessions with a sleep(230) and can confirm that the session is getting locked and cannot do anything in the site with the same browser (aka, same session). Both same user and different user in another browser (aka, different sessions) worked ok. LOL, waiting the 230 secs to expire… Perfect, now can continue using the site. So I'd say memcached locking is working… if that was the only problem… I'd propose to pass this. Any objection? Side comment: Petr, it would be great to have some way to know, from the web UI, which session handler is being used (footer?). Having that only available via config.php and without any feedback… it's a bit like Saint Thomas.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          LOL, I should look before asking, the session handler in use is already in the footer. Perfect thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - LOL, I should look before asking, the session handler in use is already in the footer. Perfect thanks!
          Hide
          Kris Stokking added a comment -

          Michael de Raadt - If you're having an issue with locking, it could be because you're using a buggy version of the PECL extension. Are you using 2.0.0 by chance?

          Petr Škoda - Could you change this line to read 2.0.1 instead of just 2.0? It's an important difference. See https://bugs.php.net/bug.php?id=59641 for more info.

          Show
          Kris Stokking added a comment - Michael de Raadt - If you're having an issue with locking, it could be because you're using a buggy version of the PECL extension. Are you using 2.0.0 by chance? Petr Škoda - Could you change this line to read 2.0.1 instead of just 2.0? It's an important difference. See https://bugs.php.net/bug.php?id=59641 for more info.
          Hide
          Petr Škoda added a comment -

          Kris Stokking: https://bugs.php.net/bug.php?id=59641 should not affect moodle, because we have the new custom session expiration code. We are very close to the weekly, I suppose we can not change anything there now. There will be most probably other tweaks necessary and docs improvements, we can change the requirement at the same time if necessary.

          Show
          Petr Škoda added a comment - Kris Stokking : https://bugs.php.net/bug.php?id=59641 should not affect moodle, because we have the new custom session expiration code. We are very close to the weekly, I suppose we can not change anything there now. There will be most probably other tweaks necessary and docs improvements, we can change the requirement at the same time if necessary.
          Hide
          Kris Stokking added a comment -

          Sorry Petr, you are correct. I should wait until after my coffee kicks in to start posting.

          Michael, the only other thing I can think of is that you're using memcache (instead of memcacheD) as the PECL extension. We've had trouble getting it to lock properly although others (like Russell) have not.

          Show
          Kris Stokking added a comment - Sorry Petr, you are correct. I should wait until after my coffee kicks in to start posting. Michael, the only other thing I can think of is that you're using memcache (instead of memcacheD) as the PECL extension. We've had trouble getting it to lock properly although others (like Russell) have not.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ok, thanks all. I'm considering this tested. Please create new issues for any followup, regression, task related to this.

          Show
          Eloy Lafuente (stronk7) added a comment - Ok, thanks all. I'm considering this tested. Please create new issues for any followup, regression, task related to this.
          Hide
          Marina Glancy added a comment -

          And THANK YOU again for making Moodle better every day!

          Another weekly release has been released.

          Show
          Marina Glancy added a comment - And THANK YOU again for making Moodle better every day! Another weekly release has been released.
          Hide
          Michael de Raadt added a comment -

          Kris Stokking - I was using the memcached PHP extension (I learned a lot about the difference during this testing). I may have misunderstood how to test the locking, though (instructions were a bit scarce). I was using two browsers to test this.

          Show
          Michael de Raadt added a comment - Kris Stokking - I was using the memcached PHP extension (I learned a lot about the difference during this testing). I may have misunderstood how to test the locking, though (instructions were a bit scarce). I was using two browsers to test this.
          Hide
          Eric Merrill added a comment -

          It can't be two browsers, it has to be two windows or tabs in the same browser instance. Two separate browsers will each get their own session.

          Show
          Eric Merrill added a comment - It can't be two browsers, it has to be two windows or tabs in the same browser instance. Two separate browsers will each get their own session.

            People

            • Votes:
              20 Vote for this issue
              Watchers:
              33 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Agile