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

The condition_info class caches the user grade info too well

    Details

    • Testing Instructions:
      Hide

      You need two browser for testing, the purpose of the issue is to make sure that student does not need to re-login in order to gain access conditional items.

      Test for 2.6:

      1. Enable conditional availability on site
      2. Create a course with some graded activities, including
        1. assignment
        2. quiz that sets the grade automatically
        3. assignment with advanced grading
      3. Create a grading category and add some graded activities in it.
      4. Create other activities AND sections whose availability depends on the grades in modules and grade category total. Note: create only one grade condition in the item to test all cases separately.
      5. Enrol a student
      6. In another browser login as a student and make sure you can not see the activities and sections that have grade conditions
      7. Take and pass the quiz
      8. Make sure the activities and sections that depend on this quiz grade appear immediately.
      9. As teacher/admin in the first browser grade the student for assignment activities
      10. As student in the second browser make sure that access is immediately granted to the activities and sections that depend on assignment grades and grading category grade.

      Test for 2.5 and 2.4:

      Repeat the test but instead of "immediately" read it as "within 10 minutes". If you hack condition_info_base::get_cached_grade_score() you can decrease the timeout for testing purposes. The quiz should still grant access immediately.

      Show
      You need two browser for testing, the purpose of the issue is to make sure that student does not need to re-login in order to gain access conditional items. Test for 2.6: Enable conditional availability on site Create a course with some graded activities, including assignment quiz that sets the grade automatically assignment with advanced grading Create a grading category and add some graded activities in it. Create other activities AND sections whose availability depends on the grades in modules and grade category total. Note: create only one grade condition in the item to test all cases separately. Enrol a student In another browser login as a student and make sure you can not see the activities and sections that have grade conditions Take and pass the quiz Make sure the activities and sections that depend on this quiz grade appear immediately. As teacher/admin in the first browser grade the student for assignment activities As student in the second browser make sure that access is immediately granted to the activities and sections that depend on assignment grades and grading category grade. Test for 2.5 and 2.4: Repeat the test but instead of "immediately " read it as "within 10 minutes". If you hack condition_info_base::get_cached_grade_score() you can decrease the timeout for testing purposes. The quiz should still grant access immediately.
    • Workaround:
      Hide

      The student logs out and then back in.

      Show
      The student logs out and then back in.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-28463-master

      Description

      Basically the use case is, the student has already logged in and the condition_info::get_cached_grade_score function has already cached the student's grade information. Then, the student's grade changes (maybe the teacher manually edits their grade in the gradebook), then any availability conditions that the new grade would satisfy will not be released to the student due to the caching. The student must logout/in.

      Since the grade can change during another user's session (like the teacher editing the grade), the solution to this problem is somewhat tricky. The cache probably has to be moved out of the user's session.

      Replication steps:

      1. Log in as admin/teacher in one browser
      2. Log in as a student in another browser
      3. Create two assignments
      4. In the second, create a condition that relies on a grade value in the first
      5. Go to the first assignment and mark an assignment for the student (there is no need for a student submission)
      6. As the student, refresh the page and check if the second assignment is accessible
      7. As the student, log out and log in again
      8. As the student, check if the second assignment is accessible

      Expected result: The second assignment should be accessible after the grade is set, even before logging in again

      Actual result: The second assignment is not accessible until logging in again

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting this.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting this.
            Hide
            quen Sam Marshall added a comment -

            This is currently expected behaviour although it might need changing. Basically the intent is that if the student changes a grade (ie by completing a quiz), this takes effect immediately. If somebody else changes it, there might be a delay before it is noticed.

            There are basically two decent options for changing it without severe performance consequences (bear in mind that conditional activity calculation happens on virtually EVERY page, for the navigation block, so adding a big grade query is a bad idea):

            1) Reduce the cache delay so that the grade cache expires, say, every 10 minutes. I suspect at the moment it only checks on login as you say. I can do this change within the completion system code without any particular problem.

            2) I think for roles (but not groups) there is an existing mechanism that flushes the cache, basically there is some sort of 'time roles last changed' for context or user or something like that, which is read on EVERY request. If the value changes then it flushes the cache. I don't really think it's helpful to add a new value like this (that still makes an extra db query on every request) but maybe it would be feasible to figure out how this system works and either reuse it so that when a grade is updated it automatically works, or else at least see if we can retrieve it in the same query, or else at LEAST implement this in some way that other systems can piggy-back on later. Some sort of generic 'something changed regarding user X, which wasn't done by user X but by somebody else' time marker.

            Basically 2 has a better result but 1 I can implement more quickly without asking anybody - 2 sounds like it would need a major discussion with the lead developers.

            Opinions?

            Show
            quen Sam Marshall added a comment - This is currently expected behaviour although it might need changing. Basically the intent is that if the student changes a grade (ie by completing a quiz), this takes effect immediately. If somebody else changes it, there might be a delay before it is noticed. There are basically two decent options for changing it without severe performance consequences (bear in mind that conditional activity calculation happens on virtually EVERY page, for the navigation block, so adding a big grade query is a bad idea): 1) Reduce the cache delay so that the grade cache expires, say, every 10 minutes. I suspect at the moment it only checks on login as you say. I can do this change within the completion system code without any particular problem. 2) I think for roles (but not groups) there is an existing mechanism that flushes the cache, basically there is some sort of 'time roles last changed' for context or user or something like that, which is read on EVERY request. If the value changes then it flushes the cache. I don't really think it's helpful to add a new value like this (that still makes an extra db query on every request) but maybe it would be feasible to figure out how this system works and either reuse it so that when a grade is updated it automatically works, or else at least see if we can retrieve it in the same query, or else at LEAST implement this in some way that other systems can piggy-back on later. Some sort of generic 'something changed regarding user X, which wasn't done by user X but by somebody else' time marker. Basically 2 has a better result but 1 I can implement more quickly without asking anybody - 2 sounds like it would need a major discussion with the lead developers. Opinions?
            Hide
            bushido Mark Nielsen added a comment -

            I agree Sam, 1 sounds like a good fix to hold us over.

            I also agree that 2 should be a general system (more importantly a general caching system that can be manipulated/cleared by other users). Moodle's session is almost always being used to cache user data thus making the user session way too large (we have had problems on how to store such large sessions and still keep up with performance). Also, other caching mechanisms have been invented because of this, like modinfo field. If I recall correctly, I thought there was some sort of $CACHE variable in 1.9 - maybe something similar could be implemented in 2 with these use cases in mind and it could use multiple backends, db, session, memcache, redis, etc.

            Show
            bushido Mark Nielsen added a comment - I agree Sam, 1 sounds like a good fix to hold us over. I also agree that 2 should be a general system (more importantly a general caching system that can be manipulated/cleared by other users). Moodle's session is almost always being used to cache user data thus making the user session way too large (we have had problems on how to store such large sessions and still keep up with performance). Also, other caching mechanisms have been invented because of this, like modinfo field. If I recall correctly, I thought there was some sort of $CACHE variable in 1.9 - maybe something similar could be implemented in 2 with these use cases in mind and it could use multiple backends, db, session, memcache, redis, etc.
            Hide
            quen Sam Marshall added a comment - - edited

            I have now checked - unfortunately, accesslib caches data per context and not per user so it is going to be probably impossible to combine the systems. This may mean that it has to be implemented specially for condition caches - although I'm sure adding one db query per request to every page is not something that people currently struggling with poor moodle 2.x performance wants to hear...

            I would like to make it generic however so that it can be used by other subsystems too. For example, I think user group data suffers from the same problem (at least it used to); it was cached and didn't refresh immediately if a teacher edits groups. As groups are still simpler than accesslib stuff, that's probably an area where this could be implemented.

            Here's a proposal.

            Infrastructure

            New table user_cachedate with fields id, userid [unique index], cachedate. OR possibly use the existing cache_flags table for it. (Tim thinks the key can be generic and isn't necessarily a context - will need to see if it suits performance-wise.)

            New lib usercachelib.php

            New function user_cache::get_date() returns cachedate for current user (static cache within request) - this is the extra db query that all requests will make. We could consider moderating this e.g. to check only once per minute (so that all such changes may take 1 minute to take effect) but that might reduce the benefit slightly. (See below.)

            New functions user_cache::invalidate($userid) and user_cache::invalidate_sql($sql, $params); the latter one does UPDATE user_cachedate SET cachedate=$now WHERE userid IN ($sql).

            Usage

            In code like completion system, where it caches data e.g. in $SESSION->mycache, store variable $SESSION->mycache->cachedate. When creating the cache, set this to the value of user_cache::get_date().

            Every time using the cache, check that the value in $SESSION->mycache->cachedate is the same as the one in user_cache::get_date(). If it is not, discard the cache and reload data.

            When a change is made that affects the cache for another user (in this case that would be grade changes or completion changes) the code must call user_cache::invalidate or invalidate_sql. This should NOT usually be called for the current user; when making changes for the current user the appropriate cache should be specifically cleared or modified instead.

            Notes

            It would be possible to have different values for different caches, like one for if somebody else changes your grade and a different one for if somebody else changes your group, but this complicates the system and probably isn't necessary as both events should be fairly infrequent (in terms of the percentage of page loads they change on) so it probably doesn't hurt to invalidate everything at once.

            I haven't actually looked at the groups code but it would probably be possible to use the same mechanism for groups cache as well. There may be some potential performance gains there (i.e. there is a groups cache but some functions don't use it - if we make it more reliable, they probably can).

            Regarding whether to check this flag every request or e.g. only up to once per minute per user - before implementing this I will check a selection of OU logs to see the typical pattern of user page requests. I.e. if typically users make one page request every 30 seconds then there is not much benefit in doing this, but if they typically make 5 in a row then wait 2 minutes then make another 5 in a row, it could be worthwhile. This would allow me to check different times such as, if we set to 90 seconds would that be a big saving, or could we drop it to 45 without losing anything, etc. Our usage is probably as good a sample as any for this, it's not exactly going to be a precise science anyway.

            Timescale

            I would be happy to implement this but I can't promise a timescale at the moment because there's a lot on (we just launched our moodle 2.x system).

            Show
            quen Sam Marshall added a comment - - edited I have now checked - unfortunately, accesslib caches data per context and not per user so it is going to be probably impossible to combine the systems. This may mean that it has to be implemented specially for condition caches - although I'm sure adding one db query per request to every page is not something that people currently struggling with poor moodle 2.x performance wants to hear... I would like to make it generic however so that it can be used by other subsystems too. For example, I think user group data suffers from the same problem (at least it used to); it was cached and didn't refresh immediately if a teacher edits groups. As groups are still simpler than accesslib stuff, that's probably an area where this could be implemented. Here's a proposal. Infrastructure New table user_cachedate with fields id, userid [unique index] , cachedate. OR possibly use the existing cache_flags table for it. (Tim thinks the key can be generic and isn't necessarily a context - will need to see if it suits performance-wise.) New lib usercachelib.php New function user_cache::get_date() returns cachedate for current user (static cache within request) - this is the extra db query that all requests will make. We could consider moderating this e.g. to check only once per minute (so that all such changes may take 1 minute to take effect) but that might reduce the benefit slightly. (See below.) New functions user_cache::invalidate($userid) and user_cache::invalidate_sql($sql, $params); the latter one does UPDATE user_cachedate SET cachedate=$now WHERE userid IN ($sql). Usage In code like completion system, where it caches data e.g. in $SESSION->mycache, store variable $SESSION->mycache->cachedate. When creating the cache, set this to the value of user_cache::get_date(). Every time using the cache, check that the value in $SESSION->mycache->cachedate is the same as the one in user_cache::get_date(). If it is not, discard the cache and reload data. When a change is made that affects the cache for another user (in this case that would be grade changes or completion changes) the code must call user_cache::invalidate or invalidate_sql. This should NOT usually be called for the current user; when making changes for the current user the appropriate cache should be specifically cleared or modified instead. Notes It would be possible to have different values for different caches, like one for if somebody else changes your grade and a different one for if somebody else changes your group, but this complicates the system and probably isn't necessary as both events should be fairly infrequent (in terms of the percentage of page loads they change on) so it probably doesn't hurt to invalidate everything at once. I haven't actually looked at the groups code but it would probably be possible to use the same mechanism for groups cache as well. There may be some potential performance gains there (i.e. there is a groups cache but some functions don't use it - if we make it more reliable, they probably can). Regarding whether to check this flag every request or e.g. only up to once per minute per user - before implementing this I will check a selection of OU logs to see the typical pattern of user page requests. I.e. if typically users make one page request every 30 seconds then there is not much benefit in doing this, but if they typically make 5 in a row then wait 2 minutes then make another 5 in a row, it could be worthwhile. This would allow me to check different times such as, if we set to 90 seconds would that be a big saving, or could we drop it to 45 without losing anything, etc. Our usage is probably as good a sample as any for this, it's not exactly going to be a precise science anyway. Timescale I would be happy to implement this but I can't promise a timescale at the moment because there's a lot on (we just launched our moodle 2.x system).
            Hide
            bushido Mark Nielsen added a comment -

            It would be nice if the cache system also had various backends available. That way we could store the cache in memcache, redis, etc. Zend_Cache has a nice design with various front ends and back ends.

            Show
            bushido Mark Nielsen added a comment - It would be nice if the cache system also had various backends available. That way we could store the cache in memcache, redis, etc. Zend_Cache has a nice design with various front ends and back ends.
            Hide
            quen Sam Marshall added a comment -

            Mark: It already does have pluggable (ish) backends - Moodle sessions can be stored in file or database although there are a few things that don't work if you store them in files (while storing in database is not feasible for large systems, there's too much traffic to the harder-to-scale database component). I think you should also be able to make it store sessions in memcache and other places.

            So in summary - there is probably room for improvement regarding pluggable session backend - but that's certainly another issue.

            Show
            quen Sam Marshall added a comment - Mark: It already does have pluggable (ish) backends - Moodle sessions can be stored in file or database although there are a few things that don't work if you store them in files (while storing in database is not feasible for large systems, there's too much traffic to the harder-to-scale database component). I think you should also be able to make it store sessions in memcache and other places. So in summary - there is probably room for improvement regarding pluggable session backend - but that's certainly another issue.
            Hide
            bushido Mark Nielsen added a comment -

            The problem with using the session as a cache mechanism is that it ends up getting very very large. We were experiencing problems with being able to efficiently store and retrieve session data. Having an actual cache system would allow one to store individual cached items without bloating the session.

            Show
            bushido Mark Nielsen added a comment - The problem with using the session as a cache mechanism is that it ends up getting very very large. We were experiencing problems with being able to efficiently store and retrieve session data. Having an actual cache system would allow one to store individual cached items without bloating the session.
            Hide
            quen Sam Marshall added a comment -

            Mark: Okay, but essentially, that's the only thing the session is used for. So I think what you're arguing for is splitting up the session into individually cached items whereas it is currently a collection of several cached items.

            Basically I agree that would be nice but it could be a little difficult to do and we would have to make sure it doesn't reduce the efficacy of the cache. Like if it has to load 6 different things each request one at a time, there's a danger that could be slower than loading the session. However there could definitely be a benefit in terms of writing data if currently, PHP ends up saving the entire session content each time whereas split up, it would only need to save parts that change.

            I think it's out of scope of this bug (this proposal is not to introduce new cached data - just a way to clear existing cached data - I guess the same logic could apply wherever the cache is stored whether it's whole or in lots of parts). Is there a separate issue for it? If not, maybe you should file one...?

            Show
            quen Sam Marshall added a comment - Mark: Okay, but essentially, that's the only thing the session is used for. So I think what you're arguing for is splitting up the session into individually cached items whereas it is currently a collection of several cached items. Basically I agree that would be nice but it could be a little difficult to do and we would have to make sure it doesn't reduce the efficacy of the cache. Like if it has to load 6 different things each request one at a time, there's a danger that could be slower than loading the session. However there could definitely be a benefit in terms of writing data if currently, PHP ends up saving the entire session content each time whereas split up, it would only need to save parts that change. I think it's out of scope of this bug (this proposal is not to introduce new cached data - just a way to clear existing cached data - I guess the same logic could apply wherever the cache is stored whether it's whole or in lots of parts). Is there a separate issue for it? If not, maybe you should file one...?
            Hide
            bushido Mark Nielsen added a comment -

            Basically I agree that would be nice but it could be a little difficult to do and we would have to make sure it doesn't reduce the efficacy of the cache. Like if it has to load 6 different things each request one at a time, there's a danger that could be slower than loading the session.

            Agreed, the use of the caches by core routines would definitely need to be optimized because they are really used on just about every or every other page load.

            I think it's out of scope of this bug

            Agreed, but I thought I would mention this since you would be writing some sort of cache API that it might be a good idea to keep other caching mechanisms in mind and other uses of the cache.

            Show
            bushido Mark Nielsen added a comment - Basically I agree that would be nice but it could be a little difficult to do and we would have to make sure it doesn't reduce the efficacy of the cache. Like if it has to load 6 different things each request one at a time, there's a danger that could be slower than loading the session. Agreed, the use of the caches by core routines would definitely need to be optimized because they are really used on just about every or every other page load. I think it's out of scope of this bug Agreed, but I thought I would mention this since you would be writing some sort of cache API that it might be a good idea to keep other caching mechanisms in mind and other uses of the cache.
            Hide
            quen Sam Marshall added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            quen Sam Marshall added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            sschermer Steffen Schermer added a comment -

            Just a little hint:
            Version 2.5.1+ is also an affected Version to this problem!

            (sorry for my bad english)

            Show
            sschermer Steffen Schermer added a comment - Just a little hint: Version 2.5.1+ is also an affected Version to this problem! (sorry for my bad english)
            Hide
            marina Marina Glancy added a comment -

            Sam, can you look at my solution please. Apparently it is blocking one of QA tests now. I submit two branches:

            1) 2.4 - just simple 10 minutes lifetime of the session cache.

            2) master - converting it to MUC cache.
            2.1) Session cache with 10 minutes lifetime. Pro: fast, Con: 10 minutes lag
            2.2) Application cache (with 1 hour lifetime just in case). Pro: is reset immediately when teacher grades a user. Con: not as fast as session cache

            (To see 2.1 view master branch without the last commit)

            Show
            marina Marina Glancy added a comment - Sam, can you look at my solution please. Apparently it is blocking one of QA tests now. I submit two branches: 1) 2.4 - just simple 10 minutes lifetime of the session cache. 2) master - converting it to MUC cache. 2.1) Session cache with 10 minutes lifetime. Pro: fast, Con: 10 minutes lag 2.2) Application cache (with 1 hour lifetime just in case). Pro: is reset immediately when teacher grades a user. Con: not as fast as session cache (To see 2.1 view master branch without the last commit)
            Hide
            quen Sam Marshall added a comment -

            Thanks for doing this!

            About the 10 minute delay - the point of this cache was to save queries as this logic may run on many pages including the all-important course view page. 10 minutes is probably a reasonable compromise.

            About session vs. application cache - there are going to be very frequent writes to this cache, so a clustered memcache is probably not a good option, meaning it would have to go in shared memcache. Shared memcache is relatively slow, but if there is only a single request per page (I think that would be the case in this cache), we're probably in practice only talking about roughly a millisecond added to every page (where the course uses this feature) on a typical large setup.

            So basically it's about how much it is worth to have it update instantly (which is really good behaviour). Is it worth a millisecond per page? I think it might be. Don't think it will kill us anyhow. (The strings cache is the one that really knackers performance without using clustered memcache; that's because it makes like a hundred requests per page.)

            Overall I think I'd be in favour of the application-level cache as more 'correct'.

            Detailed review...

            Stable branch change looks fine (hardcoded time is sucky but since stable branch changes should be minimal, that's probably fine). +1

            master change comments (I looked mainly at the 'full' change):

            a)
            if (!defined('CONDITIONGRADESCACHELIFETIME')) {

            Should this really be done like this? Wouldn't a const in the class be better/simpler? I guess the define you can change at runtime, but it seems a bit sketchy, totally undocumented, and anyhow who would want to change it... Also trivial but shouldn't the constant have a name similar to the actual cache, like with 'gradecondition' in, just for consistency.

            b)
            + if (($usercache = $cache->get($userid)) === false ||
            + $usercache['lastreset'] + CONDITIONGRADESCACHELIFETIME < $now) {
            + $usercache['lastreset'] = $now;

            If the usercache is false this will cause a php warning; shouldn't you set $usercache to a new array or whatever it is.

            c)

            Trivia, some of the comments (from original, but lines modified here) don't have full stop at the end.

            d)
            + * Since 2.6 this is no longer session cache but the name of the functions was not changed.

            The function comment says it is only for use in testing, so wouldn't it be OK to change the name at this point?

            e)

            I'm not familiar with the MUC code but if you set 'ttl' in there (for the session cache) doesn't that mean you do not even need the custom define etc as moodle will automatically delete it?

            Show
            quen Sam Marshall added a comment - Thanks for doing this! About the 10 minute delay - the point of this cache was to save queries as this logic may run on many pages including the all-important course view page. 10 minutes is probably a reasonable compromise. About session vs. application cache - there are going to be very frequent writes to this cache, so a clustered memcache is probably not a good option, meaning it would have to go in shared memcache. Shared memcache is relatively slow, but if there is only a single request per page (I think that would be the case in this cache), we're probably in practice only talking about roughly a millisecond added to every page (where the course uses this feature) on a typical large setup. So basically it's about how much it is worth to have it update instantly (which is really good behaviour). Is it worth a millisecond per page? I think it might be. Don't think it will kill us anyhow. (The strings cache is the one that really knackers performance without using clustered memcache; that's because it makes like a hundred requests per page.) Overall I think I'd be in favour of the application-level cache as more 'correct'. Detailed review... Stable branch change looks fine (hardcoded time is sucky but since stable branch changes should be minimal, that's probably fine). +1 master change comments (I looked mainly at the 'full' change): a) if (!defined('CONDITIONGRADESCACHELIFETIME')) { Should this really be done like this? Wouldn't a const in the class be better/simpler? I guess the define you can change at runtime, but it seems a bit sketchy, totally undocumented, and anyhow who would want to change it... Also trivial but shouldn't the constant have a name similar to the actual cache, like with 'gradecondition' in, just for consistency. b) + if (($usercache = $cache->get($userid)) === false || + $usercache ['lastreset'] + CONDITIONGRADESCACHELIFETIME < $now) { + $usercache ['lastreset'] = $now; If the usercache is false this will cause a php warning; shouldn't you set $usercache to a new array or whatever it is. c) Trivia, some of the comments (from original, but lines modified here) don't have full stop at the end. d) + * Since 2.6 this is no longer session cache but the name of the functions was not changed. The function comment says it is only for use in testing, so wouldn't it be OK to change the name at this point? e) I'm not familiar with the MUC code but if you set 'ttl' in there (for the session cache) doesn't that mean you do not even need the custom define etc as moodle will automatically delete it?
            Hide
            marina Marina Glancy added a comment -

            Hi Sam, thanks a lot for review!

            a & e) ttl is a nice feature but it is not good here. I'll try to explain why. Using 'ttl' adds a timestamp to each stored key in the cache. The timestamp is updated every time set() is called. But our cached value in this case is an array where we can add some values at any time when we fetch them as needed. This means the ttl timestamp would be updated but the values that have already been in the grades array before that are not actually verified. Which can lead to the situation when user constantly browsing site and the oldest fetched grades are over an hour old but the timestamp is, say, 5 minutes old.

            Statement "if (!defined('CONDITIONGRADESCACHELIFETIME')) " means that somebody can overwrite it in config.php. I'm not documenting it because I don't think it is needed. We already have couple of statements like that for example MAX_MODINFO_CACHE_SIZE. It's just a possibility to tell admins how to apply quick fix if something goes wrong.

            At the same time now reviewing my own code I think that it's too complicated and maybe we should just use usual cache ttl and not bother with all those constants. It looks like updating grades always calls notify_changes() method so this ttl is a "just-in-case" anyway.

            b) thanks, I wonder how I did not get this warning myself while testing.

            c) I always love this peer review comment

            d) I'd prefer to deprecate it completely. If you are ok with application cache, I'll just replace calls to wipe_session_cache to cache::make(...)->purge() in unittests.

            Show
            marina Marina Glancy added a comment - Hi Sam, thanks a lot for review! a & e) ttl is a nice feature but it is not good here. I'll try to explain why. Using 'ttl' adds a timestamp to each stored key in the cache. The timestamp is updated every time set() is called. But our cached value in this case is an array where we can add some values at any time when we fetch them as needed. This means the ttl timestamp would be updated but the values that have already been in the grades array before that are not actually verified. Which can lead to the situation when user constantly browsing site and the oldest fetched grades are over an hour old but the timestamp is, say, 5 minutes old. Statement "if (!defined('CONDITIONGRADESCACHELIFETIME')) " means that somebody can overwrite it in config.php. I'm not documenting it because I don't think it is needed. We already have couple of statements like that for example MAX_MODINFO_CACHE_SIZE. It's just a possibility to tell admins how to apply quick fix if something goes wrong. At the same time now reviewing my own code I think that it's too complicated and maybe we should just use usual cache ttl and not bother with all those constants. It looks like updating grades always calls notify_changes() method so this ttl is a "just-in-case" anyway. b) thanks, I wonder how I did not get this warning myself while testing. c) I always love this peer review comment d) I'd prefer to deprecate it completely. If you are ok with application cache, I'll just replace calls to wipe_session_cache to cache::make(...)->purge() in unittests.
            Hide
            quen Sam Marshall added a comment -

            Thanks Marina!

            e) TTL - understood! I did wonder if it might be that.

            a) I don't think MAX_MODINFO_CACHE_SIZE is necessarily a great model (even though I have a feeling I wrote, or at least touched, that code at some point

            Unless I misunderstood I think this setting might have value if using the session cache (maybe somebody desperately wants it to update quicker whereas other people care more about potentially adding a query on every page) but if we're going for application cache which does instant updates anyway, I think it should probably just be replaced with a const as there should be no reason for anyone to change it ever short of really weird stuff - I don't think we should have admin settings (especially undocumented ones) for really weird stuff. In reality, in event of really weird stuff it would be better to manually flush the cache or something, not look up defines in the code.

            Or if there is a good reason for it then even if we're not documenting it elsewhere, imo there should be a comment in the code at that point explaining why it's definable and what the circumstances are in which you might want to define it in your config.php.

            d) Yes that sounds better!

            So, I do like this better if you change the define to a const (if using application cache) but +1 from me either way, it's a good fix to a longstanding problem, so submit for integration when ready as far as I'm concerned!

            Show
            quen Sam Marshall added a comment - Thanks Marina! e) TTL - understood! I did wonder if it might be that. a) I don't think MAX_MODINFO_CACHE_SIZE is necessarily a great model (even though I have a feeling I wrote, or at least touched, that code at some point Unless I misunderstood I think this setting might have value if using the session cache (maybe somebody desperately wants it to update quicker whereas other people care more about potentially adding a query on every page) but if we're going for application cache which does instant updates anyway, I think it should probably just be replaced with a const as there should be no reason for anyone to change it ever short of really weird stuff - I don't think we should have admin settings (especially undocumented ones) for really weird stuff. In reality, in event of really weird stuff it would be better to manually flush the cache or something, not look up defines in the code. Or if there is a good reason for it then even if we're not documenting it elsewhere, imo there should be a comment in the code at that point explaining why it's definable and what the circumstances are in which you might want to define it in your config.php. d) Yes that sounds better! So, I do like this better if you change the define to a const (if using application cache) but +1 from me either way, it's a good fix to a longstanding problem, so submit for integration when ready as far as I'm concerned!
            Hide
            marina Marina Glancy added a comment -

            Thanks Sam,
            I just use the 1-hour ttl then. Submitting for integration

            BTW I noticed the same $SESSION cache in completionlib and completion may depend on grades which will probably be similar situation there. But not going to address this in the same issue

            Show
            marina Marina Glancy added a comment - Thanks Sam, I just use the 1-hour ttl then. Submitting for integration BTW I noticed the same $SESSION cache in completionlib and completion may depend on grades which will probably be similar situation there. But not going to address this in the same issue
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Marina and Sam,

            Makes sense to me and passes all unit tests.

            Integrated to 24, 25 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks Marina and Sam, Makes sense to me and passes all unit tests. Integrated to 24, 25 and master.
            Hide
            andyjdavis Andrew Davis added a comment -

            Works as described. Passing.

            Show
            andyjdavis Andrew Davis added a comment - Works as described. Passing.
            Hide
            damyon Damyon Wiese added a comment -

            Here lies 52 bugs.
            All fixed or swept under a rug.
            If they come back one day,
            To our dismay,
            We all will feel quite un-smug.

            Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

            Show
            damyon Damyon Wiese added a comment - Here lies 52 bugs. All fixed or swept under a rug. If they come back one day, To our dismay, We all will feel quite un-smug. Thanks for the reporting/fixing/testing on this issue. It has been sent upstream.

              People

              • Votes:
                14 Vote for this issue
                Watchers:
                14 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/Nov/13