Details

    • Testing Instructions:
      Hide

      Part A

      Put the attached file (testlock.php) in your moodle root.
      Open 2 different browsers (not just windows of same browser) and load the testlock.php page in each.
      When refreshed simultaneously - one window should get a lock and either hold it for 6 seconds, or exit without releasing the lock, the other window should either timeout after 3 seconds (if the first window held the lock) or get a lock as soon as the first window finishes.

      Test this a few times and make sure both windows do not get a lock at the same time, and all attempts to get a lock either timeout after 3 seconds or succeed within that time. Also make sure that locks are auto-released at the end of the request always.

      Part B

      With Mysql as the DB and no setting for $CFG->lock_factory in config.php,

      Repeat Part A

      With Postgres as the DB and no setting for $CFG->lock_factory in config.php,

      Repeat Part A

      Set in config.php
      $CFG->lock_factory = '\core\lock\db_row_lock_factory';

      Repeat Part A

      Set in config.php
      $CFG->lock_factory = '\core\lock\file_lock_factory';

      Repeat Part A

      Run unit tests on mysql. (Just lib/tests/lock_test.php and lib/tests/lock_config_test.php)

      Run unit tests on postgres. (Just lib/tests/lock_test.php and lib/tests/lock_config_test.php)

      Show
      Part A Put the attached file (testlock.php) in your moodle root. Open 2 different browsers (not just windows of same browser) and load the testlock.php page in each. When refreshed simultaneously - one window should get a lock and either hold it for 6 seconds, or exit without releasing the lock, the other window should either timeout after 3 seconds (if the first window held the lock) or get a lock as soon as the first window finishes. Test this a few times and make sure both windows do not get a lock at the same time, and all attempts to get a lock either timeout after 3 seconds or succeed within that time. Also make sure that locks are auto-released at the end of the request always. Part B With Mysql as the DB and no setting for $CFG->lock_factory in config.php, Repeat Part A With Postgres as the DB and no setting for $CFG->lock_factory in config.php, Repeat Part A Set in config.php $CFG->lock_factory = '\core\lock\db_row_lock_factory'; Repeat Part A Set in config.php $CFG->lock_factory = '\core\lock\file_lock_factory'; Repeat Part A Run unit tests on mysql. (Just lib/tests/lock_test.php and lib/tests/lock_config_test.php) Run unit tests on postgres. (Just lib/tests/lock_test.php and lib/tests/lock_config_test.php)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_27_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-25500-master
    • Rank:
      2808

      Description

      Just that.

        Issue Links

          Activity

          Hide
          Aparup Banerjee added a comment -

          link to locking code.

          Show
          Aparup Banerjee added a comment - link to locking code.
          Hide
          Damyon Wiese added a comment -

          I need to get the meta of this issue moving so I can add document conversions to the cron.

          Show
          Damyon Wiese added a comment - I need to get the meta of this issue moving so I can add document conversions to the cron.
          Hide
          Damyon Wiese added a comment -

          Adding branches for a new locking subsystem in Moodle.

          There are implementations of locking using flock, postgres advisory locks and the mysql get_lock function.

          Will add some more tests and then send for peer review.

          Show
          Damyon Wiese added a comment - Adding branches for a new locking subsystem in Moodle. There are implementations of locking using flock, postgres advisory locks and the mysql get_lock function. Will add some more tests and then send for peer review.
          Hide
          Damyon Wiese added a comment -

          Other candidates for locking types are:

          APC, memcache, some custom db lock type for oracle, some custom db lock type for mssql.

          Does anyone use mongodb?

          Sam has an alternative file locking method in the cache file locking that may be better for windows systems.

          Show
          Damyon Wiese added a comment - Other candidates for locking types are: APC, memcache, some custom db lock type for oracle, some custom db lock type for mssql. Does anyone use mongodb? Sam has an alternative file locking method in the cache file locking that may be better for windows systems.
          Hide
          Damyon Wiese added a comment -

          Rebased and added a generic db locktype based on atomic row updates.

          Show
          Damyon Wiese added a comment - Rebased and added a generic db locktype based on atomic row updates.
          Hide
          Russell Smith added a comment -

          How does this interact with locking used in the caching space? Are we implementing locking mechanisms in two different places?

          Show
          Russell Smith added a comment - How does this interact with locking used in the caching space? Are we implementing locking mechanisms in two different places?
          Hide
          Russell Smith added a comment -

          I've only looked at this from a quick point of view.

          You can't use pg_advisory_locks with a single parameter. It will interfere with those using db sessions. Mostly on new installations as I expect lock numbers to be lower than session numbers over time. You'll need to switch to using the two parameter version.

          SELECT ... FOR UPDATE is a possibility on all databases, except for MySQL in MyISAM mode. We would have to force the lock table into InnoDB/transaction type mode for it to work. It could provide blocking and possibly non-blocking modes. It certainly handles the auto-release on transaction failure. I've not yet understood the transaction setup of Moodle, and whether you could make those locks run in a transaction or not.

          My earlier quick comment about caching (MUC) and locking. It has a locking implementation with what looks like many of the same functions to get a lock and release. Should we have one lock setup that can be used in both caching and in the more generic space? I'm not sure which is more feature rich as I've not looked at either in enough detail.

          That's it for the quick thinking.

          Show
          Russell Smith added a comment - I've only looked at this from a quick point of view. You can't use pg_advisory_locks with a single parameter. It will interfere with those using db sessions. Mostly on new installations as I expect lock numbers to be lower than session numbers over time. You'll need to switch to using the two parameter version. SELECT ... FOR UPDATE is a possibility on all databases, except for MySQL in MyISAM mode. We would have to force the lock table into InnoDB/transaction type mode for it to work. It could provide blocking and possibly non-blocking modes. It certainly handles the auto-release on transaction failure. I've not yet understood the transaction setup of Moodle, and whether you could make those locks run in a transaction or not. My earlier quick comment about caching (MUC) and locking. It has a locking implementation with what looks like many of the same functions to get a lock and release. Should we have one lock setup that can be used in both caching and in the more generic space? I'm not sure which is more feature rich as I've not looked at either in enough detail. That's it for the quick thinking.
          Hide
          Petr Škoda added a comment - - edited

          I do not like the top level /lock/* directory at all - we put stuff there mostly because we need nice urls in UI. I believe this should be just classes in \core\lock\ namespace. Also we try to not introduce new core subsystems if possible.

          I would like to use this for main upgrade, this pretty much disqualifies this as a new plugin type because it would not be available during install and upgrades from 2.2. I do not expect people will create tens of different locking classes, PHP locking is pretty limited, I suppose we can include every reasonable option in standard distribution.

          Do not hardcode permissions, always use $CFG->directorypermissions, $CFG->filepermissions.

          I think it might be better to add the general locking support to the DML drivers and use it from the lock class, the general rule is to write neutral DB code, this seems to go the other way.

          Show
          Petr Škoda added a comment - - edited I do not like the top level /lock/* directory at all - we put stuff there mostly because we need nice urls in UI. I believe this should be just classes in \core\lock\ namespace. Also we try to not introduce new core subsystems if possible. I would like to use this for main upgrade, this pretty much disqualifies this as a new plugin type because it would not be available during install and upgrades from 2.2. I do not expect people will create tens of different locking classes, PHP locking is pretty limited, I suppose we can include every reasonable option in standard distribution. Do not hardcode permissions, always use $CFG->directorypermissions, $CFG->filepermissions. I think it might be better to add the general locking support to the DML drivers and use it from the lock class, the general rule is to write neutral DB code, this seems to go the other way.
          Hide
          Damyon Wiese added a comment -

          Re: new plugin type for locks - the main reason for this is that I can imagine some lock types needing their own settings (memcache) + some of these lock types need their own database tables. Also I expect there are useful locktypes that could be added in a plugin e.g. "Google app engine magic lock".

          Also - you can't use all these lock types in the upgrade code anyway because some require DB tables be created etc.

          Re: MUC locking - the idea would be to add this locking framework and convert MUC to use it. MUC has only one lock type now and the API is compatible. I have avoided any use of MUC here for this reason.

          IMO Locking does not belong in the DML drivers - there are other types of locks that may be faster for certain users. E.g. "flock" - memcache apc.

          Select for update (is the genericdb lock type here) is the slowest of the types listed and is always blocking (no timeouts). Blocking type locks are not as good for e.g. cron where we want to see if we can get a lock, and if not - move onto the next section.

          Show
          Damyon Wiese added a comment - Re: new plugin type for locks - the main reason for this is that I can imagine some lock types needing their own settings (memcache) + some of these lock types need their own database tables. Also I expect there are useful locktypes that could be added in a plugin e.g. "Google app engine magic lock". Also - you can't use all these lock types in the upgrade code anyway because some require DB tables be created etc. Re: MUC locking - the idea would be to add this locking framework and convert MUC to use it. MUC has only one lock type now and the API is compatible. I have avoided any use of MUC here for this reason. IMO Locking does not belong in the DML drivers - there are other types of locks that may be faster for certain users. E.g. "flock" - memcache apc. Select for update (is the genericdb lock type here) is the slowest of the types listed and is always blocking (no timeouts). Blocking type locks are not as good for e.g. cron where we want to see if we can get a lock, and if not - move onto the next section.
          Hide
          Petr Škoda added a comment -

          Upgrades would probably benefit most from the locking, we risk serious data loss there if it runs in parallel, the current code there is not very reliable.

          Show
          Petr Škoda added a comment - Upgrades would probably benefit most from the locking, we risk serious data loss there if it runs in parallel, the current code there is not very reliable.
          Hide
          Damyon Wiese added a comment -

          Re: "google app engine magic lock" - after reading a bit - I think memcache locks would be suitable for most exotic setups - so if we add that as a lock type and just not make it pluggable it should still meet everyones needs.

          Re: putting the lock code in the DML driver - I think I get what you mean - we could have the genericdb lock type in the base class and the DB specific implementations in their own driver classes and then just have one lock type "DB lock" that covers all.

          Re: locking during upgrade - we could add a special lock type just for this use case that is based on the genericdb lock - but uses the config table to store the lock.

          Show
          Damyon Wiese added a comment - Re: "google app engine magic lock" - after reading a bit - I think memcache locks would be suitable for most exotic setups - so if we add that as a lock type and just not make it pluggable it should still meet everyones needs. Re: putting the lock code in the DML driver - I think I get what you mean - we could have the genericdb lock type in the base class and the DB specific implementations in their own driver classes and then just have one lock type "DB lock" that covers all. Re: locking during upgrade - we could add a special lock type just for this use case that is based on the genericdb lock - but uses the config table to store the lock.
          Hide
          Damyon Wiese added a comment -

          Rebased,
          Moved the DB specific lock code to the DML drivers,
          Removed the plugin type for locks
          Moved the lock classes to /lib/classes/lock (namespace \core\lock)

          Not done:
          Special handling for using locks on upgrade
          Memcache lock support.

          Show
          Damyon Wiese added a comment - Rebased, Moved the DB specific lock code to the DML drivers, Removed the plugin type for locks Moved the lock classes to /lib/classes/lock (namespace \core\lock) Not done: Special handling for using locks on upgrade Memcache lock support.
          Hide
          Damyon Wiese added a comment -

          Updated again.

          I wrote a dirty feeling special lock type for upgrades that does row level locking using the config_plugins table. Welcome thoughts and suggestions on it.

          I also wrote a memcache lock type.

          Show
          Damyon Wiese added a comment - Updated again. I wrote a dirty feeling special lock type for upgrades that does row level locking using the config_plugins table. Welcome thoughts and suggestions on it. I also wrote a memcache lock type.
          Hide
          Damyon Wiese added a comment -

          Hmm just spotted that require_once($CFG->libdir.'/pluginlib.php') which should be removed from admin/settings/server.php (but I'm not going to rebase 4 branches for that tonight).

          Show
          Damyon Wiese added a comment - Hmm just spotted that require_once($CFG->libdir.'/pluginlib.php') which should be removed from admin/settings/server.php (but I'm not going to rebase 4 branches for that tonight).
          Hide
          Damyon Wiese added a comment -

          Note for integrator: I bumped the $lastmajordbchanges in case you need to resolve conflicts on version.php

          Show
          Damyon Wiese added a comment - Note for integrator: I bumped the $lastmajordbchanges in case you need to resolve conflicts on version.php
          Hide
          Damyon Wiese added a comment -

          Fred is interested in this for memcache session locks so - I've rebased this - removed the line mentioned above and cleaned up all valid codechecker/moodlecheck warnings. (There are some invalid warnings from moodlecheck saying that the lock category is not valid - that's because I am adding it).

          Show
          Damyon Wiese added a comment - Fred is interested in this for memcache session locks so - I've rebased this - removed the line mentioned above and cleaned up all valid codechecker/moodlecheck warnings. (There are some invalid warnings from moodlecheck saying that the lock category is not valid - that's because I am adding it).
          Hide
          Damyon Wiese added a comment -

          Posting a chat log about this issue:

          (09:54:41) Fred: The manager doesn't allow partners to create new locks
          (09:54:57) Damyon: It does with a small hackery
          (09:55:08) Fred: Yes, if in the right directory
          (09:55:50) Fred: I guess I would have allowed $CFG->locktype to contain a namespace'd class
          (09:55:54) Fred: So it could be anywhere
          (09:56:31) Fred: Anyway, not reviewing this, at least not now
          (09:56:33) Damyon: mmm - thats a good idea
          (09:57:11) Fred: Perhaps a class_exists() with throw new coding_exception too?
          (09:57:12) Damyon: any suggestions for get_all_lock_types
          (09:57:30) Fred: Yes, better indentation :D
          (09:57:43) Damyon: to make it dynamic without requiring full plugins
          (09:57:58) Fred: get_plugins_with_class?
          (09:58:22) Damyon: no - they are not plugins
          (09:58:23) Fred: The result of that is cached
          (09:58:30) Fred: They would be in a plugin
          (09:58:33) Fred: Oh right, sorry
          (09:58:54) Damyon: they were and I removed it based on Petrs comments
          (09:59:16) Fred: Why do you need the list in the first place?
          (09:59:24) Fred: Oh, admin setting...
          (09:59:29) Damyon: so you can choose them in the admin settings
          (09:59:48) Fred: But you cannot really chose them in the admin if set in CFG->locktype
          (10:00:08) Damyon: no - it will be locked
          (10:00:26) Damyon: so - thirdparty could just add their class and hardcode it in config.php
          (10:01:01) Fred: And if they do it's locked, so no point in choosing any.
          (10:01:46) Fred: Well... I guess it's not necessary to have a whole extensive stuff for that, if they add to config.php, then it's locked period
          (10:02:07) Fred: And you display the hardcoded name in the admin settings
          (10:02:15) Fred: Otherwise, they can choose
          (10:02:39) Damyon: yep - I'll change it to be a full namespaced class in config.php and detect this in the settings.
          (10:03:22) Fred: I think you should fallback on another lock if CFG->memcachelockservers is not properly set
          (10:03:34) Fred: Otherwise they could lock themselves out as I'm going to use that for sessions
          (10:04:07) Damyon: I just changed that from not allowing you to choose it if it wasn't set - but then you had to go to the setting page twice
          (10:04:09) Fred: Hum, you're actually handling it
          (10:04:42) Fred: Perhaps a debugging message then...
          (10:05:39) Fred: get_current_lock_type() could call is_available which checks if the rquirements are met, and returns a safe lock if not
          (10:05:49) Fred: I think we need a lock that will always work, even if dummy
          (10:06:10) Damyon: That is dangerous - no deal
          (10:06:19) Damyon: process1 asks for a lock
          (10:06:26) Damyon: gets a "safe lock"
          (10:06:31) Damyon: process2 asks for a lock
          (10:06:35) Damyon: conditions have changed
          (10:06:40) Damyon: gets a memcache lock
          (10:06:43) Damyon: boom
          (10:06:55) Fred: I see
          (10:06:56) Fred: Good point
          (10:07:20) Fred: Maybe the dummy could return false all the time
          (10:07:27) Fred: So that you never acquire a lock
          (10:08:16) Damyon: what would happen to sessions?
          

          Result is we decided that you should not be able to change the lock type from the admin settings if it may be used for sessions because you may lock yourself out. Also changing a lock type should require a server restart in order to clear all locks. Finally the $CFG->locktype should be a full namespaced class in order for plugins to add a new lock type in a different location and set it directly in the config file.

          Will add locktype to config-dist.php.
          Will change $CFG->locktype to full namespaced class name
          Will remove admin settings for changing the locktype.
          Will also remove the admin settings for the memcache locktype and only allow it to be set in the config file (and put it in config-dist.php).

          Show
          Damyon Wiese added a comment - Posting a chat log about this issue: (09:54:41) Fred: The manager doesn't allow partners to create new locks (09:54:57) Damyon: It does with a small hackery (09:55:08) Fred: Yes, if in the right directory (09:55:50) Fred: I guess I would have allowed $CFG->locktype to contain a namespace'd class (09:55:54) Fred: So it could be anywhere (09:56:31) Fred: Anyway, not reviewing this , at least not now (09:56:33) Damyon: mmm - thats a good idea (09:57:11) Fred: Perhaps a class_exists() with throw new coding_exception too? (09:57:12) Damyon: any suggestions for get_all_lock_types (09:57:30) Fred: Yes, better indentation :D (09:57:43) Damyon: to make it dynamic without requiring full plugins (09:57:58) Fred: get_plugins_with_class? (09:58:22) Damyon: no - they are not plugins (09:58:23) Fred: The result of that is cached (09:58:30) Fred: They would be in a plugin (09:58:33) Fred: Oh right, sorry (09:58:54) Damyon: they were and I removed it based on Petrs comments (09:59:16) Fred: Why do you need the list in the first place? (09:59:24) Fred: Oh, admin setting... (09:59:29) Damyon: so you can choose them in the admin settings (09:59:48) Fred: But you cannot really chose them in the admin if set in CFG->locktype (10:00:08) Damyon: no - it will be locked (10:00:26) Damyon: so - thirdparty could just add their class and hardcode it in config.php (10:01:01) Fred: And if they do it's locked, so no point in choosing any. (10:01:46) Fred: Well... I guess it's not necessary to have a whole extensive stuff for that, if they add to config.php, then it's locked period (10:02:07) Fred: And you display the hardcoded name in the admin settings (10:02:15) Fred: Otherwise, they can choose (10:02:39) Damyon: yep - I'll change it to be a full namespaced class in config.php and detect this in the settings. (10:03:22) Fred: I think you should fallback on another lock if CFG->memcachelockservers is not properly set (10:03:34) Fred: Otherwise they could lock themselves out as I'm going to use that for sessions (10:04:07) Damyon: I just changed that from not allowing you to choose it if it wasn't set - but then you had to go to the setting page twice (10:04:09) Fred: Hum, you're actually handling it (10:04:42) Fred: Perhaps a debugging message then... (10:05:39) Fred: get_current_lock_type() could call is_available which checks if the rquirements are met, and returns a safe lock if not (10:05:49) Fred: I think we need a lock that will always work, even if dummy (10:06:10) Damyon: That is dangerous - no deal (10:06:19) Damyon: process1 asks for a lock (10:06:26) Damyon: gets a "safe lock" (10:06:31) Damyon: process2 asks for a lock (10:06:35) Damyon: conditions have changed (10:06:40) Damyon: gets a memcache lock (10:06:43) Damyon: boom (10:06:55) Fred: I see (10:06:56) Fred: Good point (10:07:20) Fred: Maybe the dummy could return false all the time (10:07:27) Fred: So that you never acquire a lock (10:08:16) Damyon: what would happen to sessions? Result is we decided that you should not be able to change the lock type from the admin settings if it may be used for sessions because you may lock yourself out. Also changing a lock type should require a server restart in order to clear all locks. Finally the $CFG->locktype should be a full namespaced class in order for plugins to add a new lock type in a different location and set it directly in the config file. Will add locktype to config-dist.php. Will change $CFG->locktype to full namespaced class name Will remove admin settings for changing the locktype. Will also remove the admin settings for the memcache locktype and only allow it to be set in the config file (and put it in config-dist.php).
          Hide
          Damyon Wiese added a comment -

          Repushed with all the above changes complete.

          Show
          Damyon Wiese added a comment - Repushed with all the above changes complete.
          Hide
          Frédéric Massart added a comment -

          Hi Damyon,

          here is a more complete review:

          1. When do we decide to use constants and/or config settings? Your config setting would be good as a constant, especially that it MUST not change during the process. Just a thought.
          2. #L3R62 The PHP Doc is wrong, you don't know what the return value is in is_lock_stackable().
          3. In the file lock() method, it seems to me that you would only enter the while() when on Windows (as $wouldblock would be set to true), which seems inefficient because Windows locks the process and so the usleep() function would have no gain. Did I miss something here?
          4. In file lock, shouldn't you set the lockfile back to null, instead of 0?
          5. Interface: I don't really like the name of is_blocking(). Could it be renamed to something like is_timeout_allowed() or something more readable?
          6. In memcache generate_key(), I would add a unique character between the site identifier and the resource name.
          7. In memcache, is it right to re-open a connection everytime we try to acquire a lock?
          8. In memcache unlock(), we should return the result of delete() instead of the hardcoded true.
          9. In the upgrade lock, I would name the component core_lock_upgrade, but who cares?
          10. Should generate_unique_token() be in the database API? It seems to me that it could be useful in other places.
          11. In DB lock(), why $this->get_records('lock_db')?
          12. In DB lock(), if the record does not exist, there are unnecessary queries (insert + update + count)
          13. In DB lock(), instead of counting the records, can you get the number of affected rows? Not sure our APIs support that.
          14. In PG, do you need self::$validlocktypes as you're using the same key all the time?

          Not specific

          1. In general, you could use a do {} while(); instead of duplicating part of the logic.
          2. I am concerned about the way devs are supposed to get a lock. My understanding was that I could simply instanciate a lock, and then lock as many resources as I wanted. But it seems that I can only lock on resource per instance, which I don't think anyone will think of. The API does not prevent you from locking many resources, but as soon as you do, the instance will only remember the last lock you set... I have a few suggestions:
            • Rename the manager to get a lock engine, or something more accurate than a type. Possibly accepting the resource key as first param.
            • And/or create a private constructor, and a create() static, which accepts the resource name as a param, to enforce that you're not using multiple resource with the same instance.
            • And/or possibly create an abstract class which holds the private constructor and static create().
            • As an alternative to all this, allow for multiple resources to be used within each locktype, or throw an exception when try to lock with a different resource (I don't think this is right though).
          3. I like whitespaces between keys and values in arrays . array(key => value)

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - Hi Damyon, here is a more complete review: When do we decide to use constants and/or config settings? Your config setting would be good as a constant, especially that it MUST not change during the process. Just a thought. #L3R62 The PHP Doc is wrong, you don't know what the return value is in is_lock_stackable(). In the file lock() method, it seems to me that you would only enter the while() when on Windows (as $wouldblock would be set to true), which seems inefficient because Windows locks the process and so the usleep() function would have no gain. Did I miss something here? In file lock, shouldn't you set the lockfile back to null, instead of 0? Interface: I don't really like the name of is_blocking(). Could it be renamed to something like is_timeout_allowed() or something more readable ? In memcache generate_key(), I would add a unique character between the site identifier and the resource name. In memcache, is it right to re-open a connection everytime we try to acquire a lock? In memcache unlock(), we should return the result of delete() instead of the hardcoded true. In the upgrade lock, I would name the component core_lock_upgrade, but who cares? Should generate_unique_token() be in the database API? It seems to me that it could be useful in other places. In DB lock(), why $this->get_records('lock_db')? In DB lock(), if the record does not exist, there are unnecessary queries (insert + update + count) In DB lock(), instead of counting the records, can you get the number of affected rows? Not sure our APIs support that. In PG, do you need self::$validlocktypes as you're using the same key all the time? Not specific In general, you could use a do {} while(); instead of duplicating part of the logic. I am concerned about the way devs are supposed to get a lock. My understanding was that I could simply instanciate a lock, and then lock as many resources as I wanted. But it seems that I can only lock on resource per instance, which I don't think anyone will think of. The API does not prevent you from locking many resources, but as soon as you do, the instance will only remember the last lock you set... I have a few suggestions: Rename the manager to get a lock engine, or something more accurate than a type . Possibly accepting the resource key as first param. And/or create a private constructor, and a create() static, which accepts the resource name as a param, to enforce that you're not using multiple resource with the same instance. And/or possibly create an abstract class which holds the private constructor and static create(). As an alternative to all this, allow for multiple resources to be used within each locktype, or throw an exception when try to lock with a different resource (I don't think this is right though). I like whitespaces between keys and values in arrays . array(key => value) Cheers! Fred
          Hide
          Frédéric Massart added a comment -

          One last thing that I discovered while playing with the new session framework, was that the code must not use globals, but store a reference to them in the class. The problem was that on __deconstruct(), as the end of the PHP script, the session handlers are called, but the references to $DB and $CFG are already lost. They must be referenced in the class instance.

          Quick patch:

          diff --git a/lib/classes/lock/db.php b/lib/classes/lock/db.php
          index 77450dc..167eda9 100644
          --- a/lib/classes/lock/db.php
          +++ b/lib/classes/lock/db.php
          @@ -40,6 +40,13 @@ class db implements \core\lock\locktype {
               /** @var string $token A uniq token representing a held lock */
               protected $token = '';
           
          +    protected $db;
          +
          +    public function __construct() {
          +        global $DB;
          +        $this->db = $DB;
          +    }
          +
               /**
                * Is available.
                * @return boolean - True if this lock type is available in this environment.
          @@ -53,8 +60,7 @@ class db implements \core\lock\locktype {
                * @return boolean - Defer to the DB driver.
                */
               public function is_blocking() {
          -        global $DB;
          -        return $DB->is_lock_blocking();
          +        return $this->db->is_lock_blocking();
               }
           
               /**
          @@ -62,8 +68,7 @@ class db implements \core\lock\locktype {
                * @return boolean - False
                */
               public function is_auto_released() {
          -        global $DB;
          -        return $DB->is_lock_auto_released();
          +        return $this->db->is_lock_auto_released();
               }
           
               /**
          @@ -71,8 +76,7 @@ class db implements \core\lock\locktype {
                * @return boolean - True
                */
               public function is_stackable() {
          -        global $DB;
          -        return $DB->is_lock_stackable();
          +        return $this->db->is_lock_stackable();
               }
           
               /**
          @@ -83,10 +87,7 @@ class db implements \core\lock\locktype {
                * @return boolean - true if a lock was obtained.
                */
               public function lock($resource, $timeout, $maxlifetime = 86400) {
          -        global $DB;
          -
          -        $this->token = $DB->lock($resource, $timeout, $maxlifetime);
          -
          +        $this->token = $this->db->lock($resource, $timeout, $maxlifetime);
                   return $this->token !== false;
               }
           
          @@ -95,8 +96,6 @@ class db implements \core\lock\locktype {
                * @return boolean - true if the lock is no longer held (including if it was never held).
                */
               public function unlock() {
          -        global $DB;
          -
          -        return $DB->unlock($this->token);
          +        return $this->db->unlock($this->token);
               }
           }
          
          
          Show
          Frédéric Massart added a comment - One last thing that I discovered while playing with the new session framework, was that the code must not use globals, but store a reference to them in the class. The problem was that on __deconstruct(), as the end of the PHP script, the session handlers are called, but the references to $DB and $CFG are already lost. They must be referenced in the class instance. Quick patch: diff --git a/lib/classes/lock/db.php b/lib/classes/lock/db.php index 77450dc..167eda9 100644 --- a/lib/classes/lock/db.php +++ b/lib/classes/lock/db.php @@ -40,6 +40,13 @@ class db implements \core\lock\locktype { /** @var string $token A uniq token representing a held lock */ protected $token = ''; + protected $db; + + public function __construct() { + global $DB; + $this->db = $DB; + } + /** * Is available. * @return boolean - True if this lock type is available in this environment. @@ -53,8 +60,7 @@ class db implements \core\lock\locktype { * @return boolean - Defer to the DB driver. */ public function is_blocking() { - global $DB; - return $DB->is_lock_blocking(); + return $this->db->is_lock_blocking(); } /** @@ -62,8 +68,7 @@ class db implements \core\lock\locktype { * @return boolean - False */ public function is_auto_released() { - global $DB; - return $DB->is_lock_auto_released(); + return $this->db->is_lock_auto_released(); } /** @@ -71,8 +76,7 @@ class db implements \core\lock\locktype { * @return boolean - True */ public function is_stackable() { - global $DB; - return $DB->is_lock_stackable(); + return $this->db->is_lock_stackable(); } /** @@ -83,10 +87,7 @@ class db implements \core\lock\locktype { * @return boolean - true if a lock was obtained. */ public function lock($resource, $timeout, $maxlifetime = 86400) { - global $DB; - - $this->token = $DB->lock($resource, $timeout, $maxlifetime); - + $this->token = $this->db->lock($resource, $timeout, $maxlifetime); return $this->token !== false; } @@ -95,8 +96,6 @@ class db implements \core\lock\locktype { * @return boolean - true if the lock is no longer held (including if it was never held). */ public function unlock() { - global $DB; - - return $DB->unlock($this->token); + return $this->db->unlock($this->token); } }
          Hide
          Frédéric Massart added a comment - - edited

          Another bug found while playing with this, pg_try_advisory_lock and friends return a string which is always considered 'True' by PHP. The returned values on my system were 't' for True and 'f' for False, but I guess they could be different: http://www.postgresql.org/docs/9.1/static/datatype-boolean.html

          diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php
          index 077518b..764e15c 100644
          --- a/lib/dml/pgsql_native_moodle_database.php
          +++ b/lib/dml/pgsql_native_moodle_database.php
          @@ -1421,13 +1421,15 @@ class pgsql_native_moodle_database extends moodle_database {
                   $params = array('locktype'=>self::$validlocktypes['DB_LOCK'],
                                   'index'=>$index);
                   $result = $this->get_record_sql('select pg_try_advisory_lock(:locktype, :index) AS locked', $params);
          -        $locked = (bool)($result->locked);
          +        $locked = $result->locked === 't';
          +
          +        $giveuptime = time() + $timeout;
           
                   // Try until the giveup time.
                   while (!$locked && time() < $giveuptime) {
                       usleep(rand(10000, 250000)); // Sleep between 10 and 250 milliseconds.
                       $result = $this->get_record_sql('select pg_try_advisory_lock(:locktype, :index) AS locked', $params);
          -            $locked = (bool)($result->locked);
          +            $locked = $result->locked === 't';
                   }
           
                   if ($locked) {
          @@ -1445,7 +1447,7 @@ class pgsql_native_moodle_database extends moodle_database {
                   $params = array('locktype'=>self::$validlocktypes['DB_LOCK'],
                                   'index'=>$token);
                   $result = $this->get_record_sql('select pg_advisory_unlock(:locktype, :index) AS unlocked', $params);
          -        return (bool)$result->unlocked;
          +        return $result->unlocked === 't';
               }
           
               /**
          

          Quick note: $maxlifetime doesn't seem to be used at all for PG and MySQL. Isn't that wrong to set is_lock_blocking() to false then? Maybe I am confused on the name of this function again...

          Show
          Frédéric Massart added a comment - - edited Another bug found while playing with this, pg_try_advisory_lock and friends return a string which is always considered 'True' by PHP. The returned values on my system were 't' for True and 'f' for False, but I guess they could be different: http://www.postgresql.org/docs/9.1/static/datatype-boolean.html diff --git a/lib/dml/pgsql_native_moodle_database.php b/lib/dml/pgsql_native_moodle_database.php index 077518b..764e15c 100644 --- a/lib/dml/pgsql_native_moodle_database.php +++ b/lib/dml/pgsql_native_moodle_database.php @@ -1421,13 +1421,15 @@ class pgsql_native_moodle_database extends moodle_database { $params = array('locktype'=>self::$validlocktypes['DB_LOCK'], 'index'=>$index); $result = $this->get_record_sql('select pg_try_advisory_lock(:locktype, :index) AS locked', $params); - $locked = (bool)($result->locked); + $locked = $result->locked === 't'; + + $giveuptime = time() + $timeout; // Try until the giveup time. while (!$locked && time() < $giveuptime) { usleep(rand(10000, 250000)); // Sleep between 10 and 250 milliseconds. $result = $this->get_record_sql('select pg_try_advisory_lock(:locktype, :index) AS locked', $params); - $locked = (bool)($result->locked); + $locked = $result->locked === 't'; } if ($locked) { @@ -1445,7 +1447,7 @@ class pgsql_native_moodle_database extends moodle_database { $params = array('locktype'=>self::$validlocktypes['DB_LOCK'], 'index'=>$token); $result = $this->get_record_sql('select pg_advisory_unlock(:locktype, :index) AS unlocked', $params); - return (bool)$result->unlocked; + return $result->unlocked === 't'; } /** Quick note: $maxlifetime doesn't seem to be used at all for PG and MySQL. Isn't that wrong to set is_lock_blocking() to false then? Maybe I am confused on the name of this function again...
          Hide
          Damyon Wiese added a comment -

          Thanks Fred, this is a great review so far - I'll try and address these on Wednesday.

          Show
          Damyon Wiese added a comment - Thanks Fred, this is a great review so far - I'll try and address these on Wednesday.
          Hide
          Damyon Wiese added a comment -

          Just listing some additional points raised when chatting about this.

          Petr is keen to allow different locking configuration for different things - e.g. one type of locks for sessions and a different type for cron, cache, general use etc.

          The current suggestion for this is to change the lock manager to return different locks for session, cache, cron, and general use. These would all be configurable via the config and if not set in config then the default lock type would be returned. We still don't want to allow changing the lock types on the fly - so only changing directly in config will be supported (and should be done with restarting the webserver on all nodes to clear all open locks).

          Show
          Damyon Wiese added a comment - Just listing some additional points raised when chatting about this. Petr is keen to allow different locking configuration for different things - e.g. one type of locks for sessions and a different type for cron, cache, general use etc. The current suggestion for this is to change the lock manager to return different locks for session, cache, cron, and general use. These would all be configurable via the config and if not set in config then the default lock type would be returned. We still don't want to allow changing the lock types on the fly - so only changing directly in config will be supported (and should be done with restarting the webserver on all nodes to clear all open locks).
          Hide
          Damyon Wiese added a comment -

          Rebased and pushed an update fixing alot (but not all of these). I have not done some of the bigger refactor type issues yet.

          Responses to Freds comments above:

          FROM here is a more complete review:

          1. It is a config value because it can be configured to point to one of the other lock types. We are adding changes for this anyway so it can be different for different uses of locking (e.g. session).
          2. Fixed
          3. The LOCK_DB option to flock makes it return immediately if it can't get a lock (on real OSes anyway). So we try and get a lock, if it fails - we need to keep retrying until $timeout has passed. The random sleeps are there to reduce contention when multiple process are waiting for a lock and it suddenly becomes available.
          4. Done.
          5. Changed to "supports_timeout". Tried to improve the other function names too:
          is_stackable -> supports_recursion
          is_auto_released -> supports_auto_release
          6. Done.
          7. Stil to do.
          8. Done.
          9. Disagree (it is in a lock namespace). that thinking leads to core_locking_locktype_lock_upgrade madness . That is one thing I like about ruby.
          10. We can move it later if required. There are different levels of requirements for uniqueness - this one REALLY, REALLY, REALLY needs to be unique - others the consequences are not so bad so they can be faster.
          11. No idea - thanks - removed.
          12. No - they are not unnessary - they are there to prevent race conditions if two process attempt to lock a resource that has no key. The only atomic operation here is the "update .. where".
          13. No - I don't think we support that either.
          14. It is there to document the pg functionality and provide hints to developers so they do not design some other system using pg advisory locks with overlapping keys. Interestingly - I didn't use the single integer form because session locks are already using it - if you are removing this then we could switch to that as it would allow a greater number of unique keys. But IMO that should be done in the same patch that removes the current session locking. (Chickens, eggs).

          FROM Not specific:

          1. Done
          2. Still to do.
          3. Done

          Two reported bugs:
          Fixed

          Quick note: Yes confused I think. Hopefully the function renames will help.

          My note about separate lock types in config - Still to do.

          Show
          Damyon Wiese added a comment - Rebased and pushed an update fixing alot (but not all of these). I have not done some of the bigger refactor type issues yet. Responses to Freds comments above: FROM here is a more complete review: 1. It is a config value because it can be configured to point to one of the other lock types. We are adding changes for this anyway so it can be different for different uses of locking (e.g. session). 2. Fixed 3. The LOCK_DB option to flock makes it return immediately if it can't get a lock (on real OSes anyway). So we try and get a lock, if it fails - we need to keep retrying until $timeout has passed. The random sleeps are there to reduce contention when multiple process are waiting for a lock and it suddenly becomes available. 4. Done. 5. Changed to "supports_timeout". Tried to improve the other function names too: is_stackable -> supports_recursion is_auto_released -> supports_auto_release 6. Done. 7. Stil to do. 8. Done. 9. Disagree (it is in a lock namespace). that thinking leads to core_locking_locktype_lock_upgrade madness . That is one thing I like about ruby. 10. We can move it later if required. There are different levels of requirements for uniqueness - this one REALLY, REALLY, REALLY needs to be unique - others the consequences are not so bad so they can be faster. 11. No idea - thanks - removed. 12. No - they are not unnessary - they are there to prevent race conditions if two process attempt to lock a resource that has no key. The only atomic operation here is the "update .. where". 13. No - I don't think we support that either. 14. It is there to document the pg functionality and provide hints to developers so they do not design some other system using pg advisory locks with overlapping keys. Interestingly - I didn't use the single integer form because session locks are already using it - if you are removing this then we could switch to that as it would allow a greater number of unique keys. But IMO that should be done in the same patch that removes the current session locking. (Chickens, eggs). FROM Not specific: 1. Done 2. Still to do. 3. Done Two reported bugs: Fixed Quick note: Yes confused I think. Hopefully the function renames will help. My note about separate lock types in config - Still to do.
          Hide
          David Monllaó added a comment -

          phpunit and behat are also using it's own locking system, which should be converted to this one once is integrated

          Show
          David Monllaó added a comment - phpunit and behat are also using it's own locking system, which should be converted to this one once is integrated
          Hide
          Petr Škoda added a comment -

          I am not sure we can convert phpunit to core locking because we need to obtain the exclusive lock before initialising moodle environment

          Show
          Petr Škoda added a comment - I am not sure we can convert phpunit to core locking because we need to obtain the exclusive lock before initialising moodle environment
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I was looking at this last night, code looks good and it excites me to hear that we'll be able to convert the cache to use this instead of its own locking plugins.
          Looking at the code I thought I'd mention an issue I hit the other day in the caches file store.
          The problem rotated around NFSv3 and its file stat. We've had to make a few changes to the file store to reduce the file interaction and work around these issues.
          The change that came to mind when looking at your code here was with flock which is unreliable with NFS and there was one user who we thinking was experiencing issues due to its use in conjunction with GPFS.
          I've not looked into things fully, just sharing here so that you can (now you get why I'm excited you're tackling this area right )

          Show
          Sam Hemelryk added a comment - Hi guys, I was looking at this last night, code looks good and it excites me to hear that we'll be able to convert the cache to use this instead of its own locking plugins. Looking at the code I thought I'd mention an issue I hit the other day in the caches file store. The problem rotated around NFSv3 and its file stat. We've had to make a few changes to the file store to reduce the file interaction and work around these issues. The change that came to mind when looking at your code here was with flock which is unreliable with NFS and there was one user who we thinking was experiencing issues due to its use in conjunction with GPFS. I've not looked into things fully, just sharing here so that you can (now you get why I'm excited you're tackling this area right )
          Hide
          Kris Stokking added a comment - - edited

          A couple of concerns:

          1. Why are file locks stored under the cachedir?
          2. I'd highly recommend storing some useful data within the lock itself to identify the server and process that spawned the lock so that they may be cleaned up properly if need be. This is critical in the cases where auto_release is unsupported.

          Very excited to see locking implemented in Moodle Core API's - Keep up the good work!

          Show
          Kris Stokking added a comment - - edited A couple of concerns: Why are file locks stored under the cachedir? I'd highly recommend storing some useful data within the lock itself to identify the server and process that spawned the lock so that they may be cleaned up properly if need be. This is critical in the cases where auto_release is unsupported. Very excited to see locking implemented in Moodle Core API's - Keep up the good work!
          Hide
          Damyon Wiese added a comment -

          Thanks Kris,

          Re: 1. Because that is required to be shared storage between the nodes.
          Re: 2. lsof will do this for you.

          Show
          Damyon Wiese added a comment - Thanks Kris, Re: 1. Because that is required to be shared storage between the nodes. Re: 2. lsof will do this for you.
          Hide
          Damyon Wiese added a comment -

          Finished the refactoring after the last review.

          Now you can have separate lock configs for different uses (cron, cache, session).

          The API has been refactored to allow e.g. a Memcache lock factory to hold an open connection to the memcache server and create multiple locks from it.

          example use of the api now is:

          $lockfactory = \core\lock\lock_config::get_lock_factory('cache');
          
          $lock = $lockfactory->get_lock('catfish', 60);
          
          ... do some critical stuff
          
          $lockfactory->release_lock($lock);
          
          

          This could now do with another review.

          Show
          Damyon Wiese added a comment - Finished the refactoring after the last review. Now you can have separate lock configs for different uses (cron, cache, session). The API has been refactored to allow e.g. a Memcache lock factory to hold an open connection to the memcache server and create multiple locks from it. example use of the api now is: $lockfactory = \core\lock\lock_config::get_lock_factory('cache'); $lock = $lockfactory->get_lock('catfish', 60); ... do some critical stuff $lockfactory->release_lock($lock); This could now do with another review.
          Hide
          Kris Stokking added a comment -

          1. Right, I would agree with you that locks need to be held across all nods. The problem is that locks shouldn't be treated the same as cache, which isn't critical to keep around. You don't want your locks to disappear when purge_all_caches is called.
          2. lsof only works for files, but what about the other locking backends?

          Show
          Kris Stokking added a comment - 1. Right, I would agree with you that locks need to be held across all nods. The problem is that locks shouldn't be treated the same as cache, which isn't critical to keep around. You don't want your locks to disappear when purge_all_caches is called. 2. lsof only works for files, but what about the other locking backends?
          Hide
          Damyon Wiese added a comment -

          1. Good point about purging caches - will rethink this.
          2. First - for file locking - adding writes to the file will slow this down - which if this is used for caching is not great and I don't think there is a benefit (because you can use lsof). For the general case - keeping track of open locks etc would add overhead and would only be rarely useful in the situation when something has gone wrong. If you have to clean up held locks ever, something has gone wrong. The safest thing to do if "something has gone wrong" is shutdown all nodes, clear all locks, restart all nodes. IMO it would never be safe to try and manually clear locks.

          Show
          Damyon Wiese added a comment - 1. Good point about purging caches - will rethink this. 2. First - for file locking - adding writes to the file will slow this down - which if this is used for caching is not great and I don't think there is a benefit (because you can use lsof). For the general case - keeping track of open locks etc would add overhead and would only be rarely useful in the situation when something has gone wrong. If you have to clean up held locks ever, something has gone wrong. The safest thing to do if "something has gone wrong" is shutdown all nodes, clear all locks, restart all nodes. IMO it would never be safe to try and manually clear locks.
          Hide
          Damyon Wiese added a comment -

          Added a commit to move the lockdir out of the cachedir (It can now be configured separately for each instance of the lock_file_factory and defaults to $CFG->dataroot . '/lock').

          Show
          Damyon Wiese added a comment - Added a commit to move the lockdir out of the cachedir (It can now be configured separately for each instance of the lock_file_factory and defaults to $CFG->dataroot . '/lock').
          Hide
          Kris Stokking added a comment -

          1. Thanks Damyon!
          2. Are you asking hosts and partners to force downtime whenever there's a problem with locking? I would be careful how much trust you put in that everything "should just work". That's a strong assumption that breaks down in quite a few places in Moodle. Problems happen, and those problems need to be supported. There are cases where processes can die without cleaning up locks, so you'll never know whether the lock was held by a dead process (safe to clear away) or whether it's just a long running thread (not safe to remove). This is why I made the suggestion in the first place, based on our experiences with handling locks. So yes, there's a small overhead for writing out critical information. But if that small overhead isn't happening very often (compare lock writes to cache or session writes), and can help mitigate forced downtime as well as lead to a better understanding of larger problems, IMO it's worth it. At the very least, I would request that you either run some comparison performance tests for comparison or give Partners the option to enable it for support purposes.

          Show
          Kris Stokking added a comment - 1. Thanks Damyon! 2. Are you asking hosts and partners to force downtime whenever there's a problem with locking? I would be careful how much trust you put in that everything "should just work". That's a strong assumption that breaks down in quite a few places in Moodle. Problems happen, and those problems need to be supported. There are cases where processes can die without cleaning up locks, so you'll never know whether the lock was held by a dead process (safe to clear away) or whether it's just a long running thread (not safe to remove). This is why I made the suggestion in the first place, based on our experiences with handling locks. So yes, there's a small overhead for writing out critical information. But if that small overhead isn't happening very often (compare lock writes to cache or session writes), and can help mitigate forced downtime as well as lead to a better understanding of larger problems, IMO it's worth it. At the very least, I would request that you either run some comparison performance tests for comparison or give Partners the option to enable it for support purposes.
          Hide
          js added a comment -

          Hi, as mentioned by Sam, flock() does not work with NFS based solutions. There are quite a few forums out there detailing the issues and possible workarounds. For more information, read: http://php.net/manual/en/function.flock.php

          If we want to use reliable locking for multiple nodes it will need to be with something like redis, memcached, mongodb, etc. I mention memcached and not memcache because I dont think the memcache client supports locking. We have been using redis as a locking mechanism for moodle cron (a business requirement) for a couple years now and have not had any issues.

          Show
          js added a comment - Hi, as mentioned by Sam, flock() does not work with NFS based solutions. There are quite a few forums out there detailing the issues and possible workarounds. For more information, read: http://php.net/manual/en/function.flock.php If we want to use reliable locking for multiple nodes it will need to be with something like redis, memcached, mongodb, etc. I mention memcached and not memcache because I dont think the memcache client supports locking. We have been using redis as a locking mechanism for moodle cron (a business requirement) for a couple years now and have not had any issues.
          Hide
          Damyon Wiese added a comment -

          Well - with file locking specifically - if a process dies, the lock will be released. Same goes for the DB specific locks (PG advisory locks or MySQL locking functions). But I can make an option for the lock factory to write the extra info if this is what you want.

          Re: Problems with flock - yes I am aware it sometimes sucks - that is the point of this issue, to let people configure a locking system that works with their environment. With this patch you can add a local plugin with redis locking if you like and change your config so everything uses it (or just cache/cron/whatever).

          Memcache/Memcached - For this issue it doesn't matter because both contain enough functionality to support locking - this patch currently uses memcache - but I'll add memcached because some people seem attached to it (for me it's the other way - I have seen fatal crashes with memcached but memcache always behaves nicely).

          Show
          Damyon Wiese added a comment - Well - with file locking specifically - if a process dies, the lock will be released. Same goes for the DB specific locks (PG advisory locks or MySQL locking functions). But I can make an option for the lock factory to write the extra info if this is what you want. Re: Problems with flock - yes I am aware it sometimes sucks - that is the point of this issue, to let people configure a locking system that works with their environment. With this patch you can add a local plugin with redis locking if you like and change your config so everything uses it (or just cache/cron/whatever). Memcache/Memcached - For this issue it doesn't matter because both contain enough functionality to support locking - this patch currently uses memcache - but I'll add memcached because some people seem attached to it (for me it's the other way - I have seen fatal crashes with memcached but memcache always behaves nicely).
          Hide
          Damyon Wiese added a comment -

          Pushed an update with more improvements.

          The api now looks more like this:

          $lockfactory = \core\lock\lock_config::get_lock_factory('cache');
          
          $lock = $lockfactory->get_lock('catfish', 60);
          
          ... do some critical stuff
          
          $lock->release();
          
          

          ie you only need to keep the reference to the lock - not the factory.

          Also added support for memcached and the debugging options mentioned above.

          Show
          Damyon Wiese added a comment - Pushed an update with more improvements. The api now looks more like this: $lockfactory = \core\lock\lock_config::get_lock_factory('cache'); $lock = $lockfactory->get_lock('catfish', 60); ... do some critical stuff $lock->release(); ie you only need to keep the reference to the lock - not the factory. Also added support for memcached and the debugging options mentioned above.
          Hide
          Damyon Wiese added a comment -

          (and rebased and squashed)

          Show
          Damyon Wiese added a comment - (and rebased and squashed)
          Hide
          Kris Stokking added a comment -

          Also added support for memcached and the debugging options mentioned above.

          Awesome, thanks very much Damyon!

          Show
          Kris Stokking added a comment - Also added support for memcached and the debugging options mentioned above. Awesome, thanks very much Damyon!
          Hide
          Frédéric Massart added a comment -

          Hi Damyon,

          this is definitely my last review .

          1. Missing doc for Memcached in config-dist.php
          2. I noticed that PostgreSQL requires 2 queries to acquire a lock, or 3 if it's a new key. Could that impact performances?
          3. I like the general design, just a few comments:
            • Should the factories for one type be a singleton? Is there a need to reconstruct a factory?
            • I am not seduced by the name of lock_config. To me it feels more like the factory, which would return a lock_engine, which produces a lock object. Maybe it's just me .
          4. Why is debug_info() private? Why not protected?
          5. class lock, double empty line (74-75)
          6. Is it necessary to open the connexion to memcache(d) as soon as we construct the factory? Can't that be done at the time where a lock is acquired or release?
          7. Should we automatically release non-auto released locks? In case of exception or something of that sort? I'm afraid they would never be deleted, and the design doesn't allow to create a dummy lock just to call release on it.

          That's it! Great work, this is looking really good .

          Show
          Frédéric Massart added a comment - Hi Damyon, this is definitely my last review . Missing doc for Memcached in config-dist.php I noticed that PostgreSQL requires 2 queries to acquire a lock, or 3 if it's a new key. Could that impact performances? I like the general design, just a few comments: Should the factories for one type be a singleton? Is there a need to reconstruct a factory? I am not seduced by the name of lock_config. To me it feels more like the factory, which would return a lock_engine, which produces a lock object. Maybe it's just me . Why is debug_info() private? Why not protected? class lock, double empty line (74-75) Is it necessary to open the connexion to memcache(d) as soon as we construct the factory? Can't that be done at the time where a lock is acquired or release? Should we automatically release non-auto released locks? In case of exception or something of that sort? I'm afraid they would never be deleted, and the design doesn't allow to create a dummy lock just to call release on it. That's it! Great work, this is looking really good .
          Hide
          Damyon Wiese added a comment -

          Thanks for the last review Fred.

          1. Fixed
          2. Not fixed - really it is hard to map from a string key to an integer in a way that will work across nodes. This is a DB lock type - so it uses the DB to sync the keys. Note - that it will require - 0 queries if the key is in the static cache, 1 query if the key has been created before, 2 if it is a new key that has not been seen - and 3 on rare occasions when there is a race. The try_advisory lock queries don't count really - they are shared memory only.
          3. This is an abstract factory pattern down to a tee (I thought quite hard about it)
          4. Change to protected.
          5. Removed.
          6. Changed to lazy open the connection when it's used.
          7. No - I don't think so. It's up to the calling code to use a proper try/catch block or handle this itself. The cachelock example I created does this.

          I pushed a new branch with all these changes and rebased it.

          Show
          Damyon Wiese added a comment - Thanks for the last review Fred. 1. Fixed 2. Not fixed - really it is hard to map from a string key to an integer in a way that will work across nodes. This is a DB lock type - so it uses the DB to sync the keys. Note - that it will require - 0 queries if the key is in the static cache, 1 query if the key has been created before, 2 if it is a new key that has not been seen - and 3 on rare occasions when there is a race. The try_advisory lock queries don't count really - they are shared memory only. 3. This is an abstract factory pattern down to a tee (I thought quite hard about it) 4. Change to protected. 5. Removed. 6. Changed to lazy open the connection when it's used. 7. No - I don't think so. It's up to the calling code to use a proper try/catch block or handle this itself. The cachelock example I created does this. I pushed a new branch with all these changes and rebased it.
          Hide
          Damyon Wiese added a comment -

          I'll see if Petr has any more comments tomorrow and if not I'll push this as I think it's ready.

          Show
          Damyon Wiese added a comment - I'll see if Petr has any more comments tomorrow and if not I'll push this as I think it's ready.
          Hide
          Petr Škoda added a comment -

          Hi a few more comments before this gets submitted for integration:

          1/ Settings - it is strongly discouraged to use arrays for settings because it makes it incompatible with web UI settings, the arrays are ok for stuff that is necessary before opening database connection - this is not the case here, the locking should be used for application locks (after inclusion of config.php). I am proposing to use something similar to sessions - $CFG->lock_factory_class = 'aa/bb/cc', this would allow people to create locking classes as new addons. Each locking factory would custom settings. The default would be selected based on OS, DB type, $CFG->preventfilelocking, etc.

          2/ The SQL query syntax is non-standard - see the examples in coding style guide and DML docs

          3/ I think we should remove all traces of locking from the MUC - it was not intended to be a general locking framework, the locking was added there only because of the expected usage in session code, but session did not need it in the end. The proposed locking mechanism are unusable in MUC due to limited performance. We have discussed this with Sam and got his +1. This would be a separate new issue of course.

          4/ I think the upgrade lock can be removed from this commit issue, maybe we could abstract this on different level such as in ugpradelib.php without creating special upgrade locking class.

          Show
          Petr Škoda added a comment - Hi a few more comments before this gets submitted for integration: 1/ Settings - it is strongly discouraged to use arrays for settings because it makes it incompatible with web UI settings, the arrays are ok for stuff that is necessary before opening database connection - this is not the case here, the locking should be used for application locks (after inclusion of config.php). I am proposing to use something similar to sessions - $CFG->lock_factory_class = 'aa/bb/cc', this would allow people to create locking classes as new addons. Each locking factory would custom settings. The default would be selected based on OS, DB type, $CFG->preventfilelocking, etc. 2/ The SQL query syntax is non-standard - see the examples in coding style guide and DML docs 3/ I think we should remove all traces of locking from the MUC - it was not intended to be a general locking framework, the locking was added there only because of the expected usage in session code, but session did not need it in the end. The proposed locking mechanism are unusable in MUC due to limited performance. We have discussed this with Sam and got his +1. This would be a separate new issue of course. 4/ I think the upgrade lock can be removed from this commit issue, maybe we could abstract this on different level such as in ugpradelib.php without creating special upgrade locking class.
          Hide
          Damyon Wiese added a comment -

          Thanks - I'll get this patch updated tomorrow.

          Show
          Damyon Wiese added a comment - Thanks - I'll get this patch updated tomorrow.
          Hide
          Damyon Wiese added a comment -

          1/ Settings - Changed this to a single setting $CFG->lock_factory which can be configured in the admin settings to one of the known lock types. You could configure it in the config file to something exotic if you wanted.
          The selected lock factory can also add it's own settings to the admin tree (done for memcache).

          2/ The SQL query syntax is non-standard - fixed indentation/capitalisation.

          3/ I think we should remove all traces of locking from the MUC - yep - separate issue though.

          4/ I think the upgrade lock can be removed from this commit issue, - done.

          Show
          Damyon Wiese added a comment - 1/ Settings - Changed this to a single setting $CFG->lock_factory which can be configured in the admin settings to one of the known lock types. You could configure it in the config file to something exotic if you wanted. The selected lock factory can also add it's own settings to the admin tree (done for memcache). 2/ The SQL query syntax is non-standard - fixed indentation/capitalisation. 3/ I think we should remove all traces of locking from the MUC - yep - separate issue though. 4/ I think the upgrade lock can be removed from this commit issue, - done.
          Hide
          Frédéric Massart added a comment - - edited

          Hi Damyon,

          I have reviewed that quite a bit in the past, and I suspect that this has not changed much, so I am just throwing a glance at this.

          1. The PHP Doc for lock_factory::get_lock() is confusing me: @return \core\lock\lock - True if a lock was obtained. It wouldn't be True if it was a lock object.
          2. In some factories: if (\debugging('', DEBUG_DEVELOPER))
            • You do not need to escape the function
            • We now have a new way to do this: if ($CFG->debugdeveloper), see lib/upgrade.txt:28
          3. I thought we would not have any admin setting, to prevent the loss of all the locks
          4. PHP Doc for lock_config does not seem right
          5. I am surprised that we are back to having settings in the admin... what happens if my cron is running and I decide to change my locking settings? Either the locking engine, or the lock settings? Another cron run would not know what has been locked yet.
            • Also, for Memcached I have some new settings popping up in the page after saving. I much preferred the config-dist.php way. A bit more advanced but much more secure.
          6. Missing @param in lock_config::get_lock_factory()
          7. The test file should be updated.

          That's it for me! Feel free to push this issue for integration whenever you like.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - - edited Hi Damyon, I have reviewed that quite a bit in the past, and I suspect that this has not changed much, so I am just throwing a glance at this. The PHP Doc for lock_factory::get_lock() is confusing me: @return \core\lock\lock - True if a lock was obtained. It wouldn't be True if it was a lock object. In some factories: if (\debugging('', DEBUG_DEVELOPER)) You do not need to escape the function We now have a new way to do this: if ($CFG->debugdeveloper), see lib/upgrade.txt:28 I thought we would not have any admin setting, to prevent the loss of all the locks PHP Doc for lock_config does not seem right I am surprised that we are back to having settings in the admin... what happens if my cron is running and I decide to change my locking settings? Either the locking engine, or the lock settings? Another cron run would not know what has been locked yet. Also, for Memcached I have some new settings popping up in the page after saving. I much preferred the config-dist.php way. A bit more advanced but much more secure. Missing @param in lock_config::get_lock_factory() The test file should be updated. That's it for me! Feel free to push this issue for integration whenever you like. Cheers, Fred
          Hide
          Damyon Wiese added a comment -

          Thanks for looking again Fred,

          Have been talking with Petr about this - I forgot the reason we didn't create admin settings pages for this the first time - but now that you reminded me it seems a good reason. I'll switch this back to settings in the config file only - but remove the use of arrays (no real need to have different instances of the locking anyway).

          I'll address these other things too.

          Show
          Damyon Wiese added a comment - Thanks for looking again Fred, Have been talking with Petr about this - I forgot the reason we didn't create admin settings pages for this the first time - but now that you reminded me it seems a good reason. I'll switch this back to settings in the config file only - but remove the use of arrays (no real need to have different instances of the locking anyway). I'll address these other things too.
          Hide
          Damyon Wiese added a comment -

          All done - thanks Fred and Petr - pushing for integration.

          Show
          Damyon Wiese added a comment - All done - thanks Fred and Petr - pushing for integration.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, nice stuff. Some comments:

          0) it's conflicting right now. No problem, just notting it includes upgrade code that should be kept on sync with the version.

          1) tiny detail, when you apply the "core\lock" namespace to a whole file... then it should not be needed to add any "\core\lock" prefix along the class, isn't it?

          2) if all this stuff is not a subsystem then you cannot use it as @package. If it's a subsystem, then it should be core_lock. If, instead, it's a @category (aka, an API), then it should be documented @ http://docs.moodle.org/dev/Core_APIs . Right now there are occurrences in code as package and as category. Need fixing.

          3) Got it merged and prechecked here: http://ci.stronk7.com/view/prehecker/job/Precheck%20remote%20branch/1164/artifact/work/smurf.html

          Now the important things:

          A - Are you sure that file locks are released when the process ends? It sounds to me that it changed at some point in 5.x and closing the handle does not release the lock anymore. So I'm not sure if we can rely on that for file locks. Note perhaps I'm wrong, it just sounds to me.

          B - In the database drivers... why do you need such complex tokens? At which point the id is not enough to achieve the token/owner thingy?

          C - More about databases, and this is my main point against current implementation. Why the hell are we bloating the DML drivers with methods that are not DML at all?

          IMO that's simply and absolutely, unacceptable. +10^6 to keep all that code out from DML. You can use database facilities, of course, but implement all that stuff by extending db_lock_factory and 100% out from moodle_database.

          More yet, why are we assuming that the Moodle database (that uses to be pretty busy) is the best election to run the locking system on? The same we connect to memcache[d] servers... why cannot we follow exactly the same approach for database locks? I'm ok if it defaults to Moodle database, for commodity, np with that, but can imagine using a separate connection for that, why not? Your main DB can be Mysql and (given that the locks there are really particular), you may decide to use another's database advisory locks. Moving the stuff apart from moodle_database will help for that, sure. And also will help to, in an isolated way, create locks for other databases.

          And this is my opinion. Of course, we need to be really careful about where we want all these locks working, some of them are really expensive and we should not use them for anything but critical situations like cron, upgrade... I suppose that's crystal clear and should be specified in the documentation.

          I don't want to imagine, for example, all the zillion MUC accesses fighting at some moment for a lock (using current super-expensive db implementation), each one performing 2-3 queries at the same time, every few milliseconds. Anyway, the MUC thingy is already happening in another issue. Just clarifying the goals for this locking system: To be applied to critical, low concurrency (just anti-mistakes), situations.

          So, based in all the above, specially the hacks (changes) introduced in moodle_database (2C above)... I think this needs one more round.

          Reopening, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, nice stuff. Some comments: 0) it's conflicting right now. No problem, just notting it includes upgrade code that should be kept on sync with the version. 1) tiny detail, when you apply the "core\lock" namespace to a whole file... then it should not be needed to add any "\core\lock" prefix along the class, isn't it? 2) if all this stuff is not a subsystem then you cannot use it as @package. If it's a subsystem, then it should be core_lock. If, instead, it's a @category (aka, an API), then it should be documented @ http://docs.moodle.org/dev/Core_APIs . Right now there are occurrences in code as package and as category. Need fixing. 3) Got it merged and prechecked here: http://ci.stronk7.com/view/prehecker/job/Precheck%20remote%20branch/1164/artifact/work/smurf.html Now the important things: A - Are you sure that file locks are released when the process ends? It sounds to me that it changed at some point in 5.x and closing the handle does not release the lock anymore. So I'm not sure if we can rely on that for file locks. Note perhaps I'm wrong, it just sounds to me. B - In the database drivers... why do you need such complex tokens? At which point the id is not enough to achieve the token/owner thingy? C - More about databases, and this is my main point against current implementation. Why the hell are we bloating the DML drivers with methods that are not DML at all? IMO that's simply and absolutely, unacceptable. +10^6 to keep all that code out from DML. You can use database facilities, of course, but implement all that stuff by extending db_lock_factory and 100% out from moodle_database. More yet, why are we assuming that the Moodle database (that uses to be pretty busy) is the best election to run the locking system on? The same we connect to memcache [d] servers... why cannot we follow exactly the same approach for database locks? I'm ok if it defaults to Moodle database, for commodity, np with that, but can imagine using a separate connection for that, why not? Your main DB can be Mysql and (given that the locks there are really particular), you may decide to use another's database advisory locks. Moving the stuff apart from moodle_database will help for that, sure. And also will help to, in an isolated way, create locks for other databases. And this is my opinion. Of course, we need to be really careful about where we want all these locks working, some of them are really expensive and we should not use them for anything but critical situations like cron, upgrade... I suppose that's crystal clear and should be specified in the documentation. I don't want to imagine, for example, all the zillion MUC accesses fighting at some moment for a lock (using current super-expensive db implementation), each one performing 2-3 queries at the same time, every few milliseconds. Anyway, the MUC thingy is already happening in another issue. Just clarifying the goals for this locking system: To be applied to critical, low concurrency (just anti-mistakes), situations. So, based in all the above, specially the hacks (changes) introduced in moodle_database (2C above)... I think this needs one more round. Reopening, ciao
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Damyon Wiese added a comment -

          Thanks Eloy - had a talk with Petr about the location for the DB specific lock functions - and we agreed it's Ok to move them out of DML and into their own namespace \core\locking\db.

          I'll work on this this week and get it resubmitted for next week.

          Re: the complex tokens - it makes the API much easier to use if you can use strings for your lock tokens - then we can use frankenstyle naming for locks and avoid conflicts without having to pre-register a list of all known lock ids (also better for third party code). postgres is the only DB not allowing string tokens for lock functions - so the mapping must be done for it. Also note - having strings for lock tokens will also aid in debugging because it is easier to find the code that created a lock.

          The only 3 planned uses for these locks currently are: cron, upgrade and third party code. MUC will have locking removed entirely.

          Show
          Damyon Wiese added a comment - Thanks Eloy - had a talk with Petr about the location for the DB specific lock functions - and we agreed it's Ok to move them out of DML and into their own namespace \core\locking\db. I'll work on this this week and get it resubmitted for next week. Re: the complex tokens - it makes the API much easier to use if you can use strings for your lock tokens - then we can use frankenstyle naming for locks and avoid conflicts without having to pre-register a list of all known lock ids (also better for third party code). postgres is the only DB not allowing string tokens for lock functions - so the mapping must be done for it. Also note - having strings for lock tokens will also aid in debugging because it is easier to find the code that created a lock. The only 3 planned uses for these locks currently are: cron, upgrade and third party code. MUC will have locking removed entirely.
          Hide
          Damyon Wiese added a comment -

          Summary of Eloys points.

          1/ (namespace) - I removed the \core\lock\ from the implements lines - it was not used anywhere else.

          2/ Changed all package/categories. The package is core and the category is lock - this is an API and I will document it on the API page once integrated.

          3/ Rebased

          A) I added auto_release handlers to all lock types to make sure the locks are release. This will make the locks e.g. for windows behave better too.

          B) There are 2 tokens here, 1 is the resource 'key' - allowing strings here gives a much nicer API - no need to globally register a list of all strings anywhere. The second is just used by the db_row_lock_factory - and is a UUID. IMO it's better to reuse a standard implementation for a universally unique identifier than to invent a new one - and these are fast to generate. This uuid generation could go in moodlelib.php - but I didn't want to hold up this patch just for adding this there.

          C) Moved out of DML layer. I added lock factories based on the db->get_dbfamily() (get_dbtype seemed better but it is not public). This means mysql and maria db use the same implementation (which works fine anyway).

          D) For external DB locking - you can add your own new lock factory for that - I think we are providing more than enough options in this patch already and this does not prevent that.

          E) Yes - in this docs, these should be described as "slow but safe" locks.

          So this seems ready for peer review again - I retested all the lock types with this new patch and all seems rosy.

          Show
          Damyon Wiese added a comment - Summary of Eloys points. 1/ (namespace) - I removed the \core\lock\ from the implements lines - it was not used anywhere else. 2/ Changed all package/categories. The package is core and the category is lock - this is an API and I will document it on the API page once integrated. 3/ Rebased A) I added auto_release handlers to all lock types to make sure the locks are release. This will make the locks e.g. for windows behave better too. B) There are 2 tokens here, 1 is the resource 'key' - allowing strings here gives a much nicer API - no need to globally register a list of all strings anywhere. The second is just used by the db_row_lock_factory - and is a UUID. IMO it's better to reuse a standard implementation for a universally unique identifier than to invent a new one - and these are fast to generate. This uuid generation could go in moodlelib.php - but I didn't want to hold up this patch just for adding this there. C) Moved out of DML layer. I added lock factories based on the db->get_dbfamily() (get_dbtype seemed better but it is not public). This means mysql and maria db use the same implementation (which works fine anyway). D) For external DB locking - you can add your own new lock factory for that - I think we are providing more than enough options in this patch already and this does not prevent that. E) Yes - in this docs, these should be described as "slow but safe" locks. So this seems ready for peer review again - I retested all the lock types with this new patch and all seems rosy.
          Hide
          Damyon Wiese added a comment -

          Made some changes requested by Petr.

          1. Does not use any variables from config in unit tests. If you want to unit test the memcache lock servers, you need to add some new defines.

          2. Locks are reset after each step in phpunit. If a lock has not been released here it will trigger an error. This is done in the destructor of the lock class, which is triggered after each step because we call gc_collect_cycles. (You can try removing a $lock->release() from the test and see it works.

          Show
          Damyon Wiese added a comment - Made some changes requested by Petr. 1. Does not use any variables from config in unit tests. If you want to unit test the memcache lock servers, you need to add some new defines. 2. Locks are reset after each step in phpunit. If a lock has not been released here it will trigger an error. This is done in the destructor of the lock class, which is triggered after each step because we call gc_collect_cycles. (You can try removing a $lock->release() from the test and see it works.
          Hide
          Damyon Wiese added a comment -

          Rebased...

          Show
          Damyon Wiese added a comment - Rebased...
          Hide
          Dan Poltawski added a comment -

          Hi Damyon,

          In decending order of priority:

          1. Trivial: Should we not be ussing debugging() rather trigger_error() in destruct()? Seems un-moodley.
          2. Important: I don't think you have any special handling for this 'auto' value mentioned in config-dist.php: $CFG->lock_factory = "auto";, seems like we should suggest an unset default? (Looks like if an 'auto' class was created, it'd try and use that?)
          3. Critical! Am I missing something... I don't see how we can call these locks 'slow but safe' and have a memcached locking implementation in core?? It seems like the completely wrong technology to use for locking! Not persistant at all and has the risk of loosing the lock after a service restart or simply filling up the cache? It really seems to me like a dedicated lock db (different from the Moodle one, like Eloy suggests) would be a better and safer apporach for big implementations there.

          Perhaps it would help if we had the clear goals of this locking system documented here - when it would be appropiate for use? Because if someone switches to a memcached locking implementation it seems to me like it'd suggest a low level of robustness in the locking system?

          (Would also like Eloy to look at it again )

          Show
          Dan Poltawski added a comment - Hi Damyon, In decending order of priority: 1. Trivial: Should we not be ussing debugging() rather trigger_error() in destruct()? Seems un-moodley. 2. Important: I don't think you have any special handling for this 'auto' value mentioned in config-dist.php: $CFG->lock_factory = "auto"; , seems like we should suggest an unset default? (Looks like if an 'auto' class was created, it'd try and use that?) 3. Critical! Am I missing something... I don't see how we can call these locks 'slow but safe' and have a memcached locking implementation in core?? It seems like the completely wrong technology to use for locking! Not persistant at all and has the risk of loosing the lock after a service restart or simply filling up the cache? It really seems to me like a dedicated lock db (different from the Moodle one, like Eloy suggests) would be a better and safer apporach for big implementations there. Perhaps it would help if we had the clear goals of this locking system documented here - when it would be appropiate for use? Because if someone switches to a memcached locking implementation it seems to me like it'd suggest a low level of robustness in the locking system? (Would also like Eloy to look at it again )
          Hide
          Damyon Wiese added a comment -

          Thanks Dan,

          Responses:

          1. debugging would not cause a unit test to fail in this case - I changed it to throw a coding_exception which is a little more moodley and still fails a unit test.
          2. I changed it to handle auto - mainly because you made me realise it is important not to silently fallback if the locking class is not available. This could lead to cluster nodes silently using the wrong locking type if there is a typo in the config.
          3. I had wanted to provide alternatives early on - because it looked like there were environments with not many good options (windows) and the uses of this locking were not clearly decided (it was talked about for MUC so performance would have been important). I think the row locking is now robust enough (supports auto-release) for these environments, and we don't care about performance, so we can probably remove these memcache varieties. Will do now.

          Added 2 commits with these changes.

          Show
          Damyon Wiese added a comment - Thanks Dan, Responses: 1. debugging would not cause a unit test to fail in this case - I changed it to throw a coding_exception which is a little more moodley and still fails a unit test. 2. I changed it to handle auto - mainly because you made me realise it is important not to silently fallback if the locking class is not available. This could lead to cluster nodes silently using the wrong locking type if there is a typo in the config. 3. I had wanted to provide alternatives early on - because it looked like there were environments with not many good options (windows) and the uses of this locking were not clearly decided (it was talked about for MUC so performance would have been important). I think the row locking is now robust enough (supports auto-release) for these environments, and we don't care about performance, so we can probably remove these memcache varieties. Will do now. Added 2 commits with these changes.
          Hide
          Dan Poltawski added a comment -

          Fred was just questioning me about this - to be clear, I think a memcached option in 'contrib' is fine for people who understand the risks to implement it. I'm just don't think we should offer it as a default option, if we are suggesting their use is for 'slow but safe' things. As Damyon's commit says "A restart of the memcache server (or out of memory) could lead to multiple locks being given for the same resource.". I don't think that is a caveat developers should have to be concerned about.

          So, in contrib sysadmins who have the information available can choose to take the risk, but developers work on the assumption that the locks are reliable.

          Show
          Dan Poltawski added a comment - Fred was just questioning me about this - to be clear, I think a memcached option in 'contrib' is fine for people who understand the risks to implement it. I'm just don't think we should offer it as a default option, if we are suggesting their use is for 'slow but safe' things. As Damyon's commit says "A restart of the memcache server (or out of memory) could lead to multiple locks being given for the same resource.". I don't think that is a caveat developers should have to be concerned about. So, in contrib sysadmins who have the information available can choose to take the risk, but developers work on the assumption that the locks are reliable.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi just some comments to sum to Dan:

          E0) some post tasks:

          • document the new api in the docs.
          • document them as “safe and slow” locks not for much concurrency and relying 100% on the status of the lock schema used that may need configuration. (memcached/db down, or expiring before expected… whatever).

          E1) Somehow related: Still I continue thinking that we should restrict the 2st level of namespaces to be some known vocabulary (and apis are sounding more and more a good one. so @category and namespace 2nd level should always match and exist as a valid API). Freedom below 2nd level. The picked “lock” here sounds ok. But we should rule that ASAP (MDLSITE-2549). This is just a good example.

          E2) Why there isn’t any implementation of those mutexes/advisory locks for mssql and oracle? They support them afaik (we have them for sessions since 2.0).

          E3) I don’t think “db_row_lock_factory” is a good name, it’s not really row locking at all (that in fact could be implemented too for some databases providing an API for row locking). It’s record-based “pseudo-locking”, IMO. I would change that name keeping it available for future uses.

          E4) About the locking factories selected and their reliability… I’m 100% neutral. I don’t think memcached is better/safer or the opposite. As commented above, we just need to document it, the ideal conf and the drawbacks in a wrong setup. I’m pretty sure 99% of people will run them in default mode.

          E5) file/class phpdocs for various lock factories should be tidied from their current copy/paste form, to give some clue about every implementation. Not critical but…

          E6) “…you need to add some new defines.” => Which defines?

          All them really minor details (but E1 - it does not allow me to sleep!, and E2 - just curiosity for your biased selection, lol).

          I like it a lot, great work!

          Show
          Eloy Lafuente (stronk7) added a comment - Hi just some comments to sum to Dan: E0) some post tasks: document the new api in the docs. document them as “safe and slow” locks not for much concurrency and relying 100% on the status of the lock schema used that may need configuration. (memcached/db down, or expiring before expected… whatever). E1) Somehow related: Still I continue thinking that we should restrict the 2st level of namespaces to be some known vocabulary (and apis are sounding more and more a good one. so @category and namespace 2nd level should always match and exist as a valid API). Freedom below 2nd level. The picked “lock” here sounds ok. But we should rule that ASAP ( MDLSITE-2549 ). This is just a good example. E2) Why there isn’t any implementation of those mutexes/advisory locks for mssql and oracle? They support them afaik (we have them for sessions since 2.0). E3) I don’t think “db_row_lock_factory” is a good name, it’s not really row locking at all (that in fact could be implemented too for some databases providing an API for row locking). It’s record-based “pseudo-locking”, IMO. I would change that name keeping it available for future uses. E4) About the locking factories selected and their reliability… I’m 100% neutral. I don’t think memcached is better/safer or the opposite. As commented above, we just need to document it, the ideal conf and the drawbacks in a wrong setup. I’m pretty sure 99% of people will run them in default mode. E5) file/class phpdocs for various lock factories should be tidied from their current copy/paste form, to give some clue about every implementation. Not critical but… E6) “…you need to add some new defines.” => Which defines? All them really minor details (but E1 - it does not allow me to sleep!, and E2 - just curiosity for your biased selection, lol). I like it a lot, great work!
          Hide
          Damyon Wiese added a comment -

          E6 doesn't count any more because I removed the memcache lock types.

          Show
          Damyon Wiese added a comment - E6 doesn't count any more because I removed the memcache lock types.
          Hide
          Dan Poltawski added a comment -

          Sorry, i'm sending it around for another iteration, it seems like it will be worthwhile..

          Show
          Dan Poltawski added a comment - Sorry, i'm sending it around for another iteration, it seems like it will be worthwhile..
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Damyon Wiese added a comment - - edited

          Rebased - and more changes:

          Responses to all of Eloys points:

          E1) Sure lets sort out the namespaces - but this implementation is 100% inline with the proposal.
          E2) I haven't implemented custom lock types for MSSQL or Oracle because of several reasons:

          • The locking code in the dml driver for each would need to be duplicated, (because we moved the locks out of the dml layer). I also don't think we should maintain that nasty code in 2 places.
          • I don't fully understand the code for the locking in those drivers and I might introduce a bug or a conflict with the session locking.
          • The db_record_locking_factory will work for those database and is simpler. The performance will be fine for the intended uses of this locking api.
          • It can be done later if anyone screams for it.

          E3) Sure - renamed to db_record_locking_factory - I suck with naming things.
          E4) I plan to add the memcache lock factories to a local plugin in the plugin DB once this is integrated (and I can add plugins for 2.7). I think this is better than maintaining them in core because of the issues around synchronising server restarts etc.
          E5) Phpdocs updated.
          E6) Mentioned before the memcache lock types are not included here, so no need for the defines.

          Pushing for integration again (Unit tests are still passing).

          Show
          Damyon Wiese added a comment - - edited Rebased - and more changes: Responses to all of Eloys points: E1) Sure lets sort out the namespaces - but this implementation is 100% inline with the proposal. E2) I haven't implemented custom lock types for MSSQL or Oracle because of several reasons: The locking code in the dml driver for each would need to be duplicated, (because we moved the locks out of the dml layer). I also don't think we should maintain that nasty code in 2 places. I don't fully understand the code for the locking in those drivers and I might introduce a bug or a conflict with the session locking. The db_record_locking_factory will work for those database and is simpler. The performance will be fine for the intended uses of this locking api. It can be done later if anyone screams for it. E3) Sure - renamed to db_record_locking_factory - I suck with naming things. E4) I plan to add the memcache lock factories to a local plugin in the plugin DB once this is integrated (and I can add plugins for 2.7). I think this is better than maintaining them in core because of the issues around synchronising server restarts etc. E5) Phpdocs updated. E6) Mentioned before the memcache lock types are not included here, so no need for the defines. Pushing for integration again (Unit tests are still passing).
          Hide
          Damyon Wiese added a comment -

          Because I think this is close enough to the final implementation - I wrote some dev docs for it and added the link here.

          Show
          Damyon Wiese added a comment - Because I think this is close enough to the final implementation - I wrote some dev docs for it and added the link here.
          Hide
          Damyon Wiese added a comment -

          I added one commit to fix a potential issue when multiple moodle instances are using the same db server on postgres. (The same issue exists with session locks - I mentioned to Petr - the impact of that will be that in rare cases - 2 users will have to take turns loading pages!)

          Also note - I have kept the last few commits all separate for reviewing - but will squash on request.

          Show
          Damyon Wiese added a comment - I added one commit to fix a potential issue when multiple moodle instances are using the same db server on postgres. (The same issue exists with session locks - I mentioned to Petr - the impact of that will be that in rare cases - 2 users will have to take turns loading pages!) Also note - I have kept the last few commits all separate for reviewing - but will squash on request.
          Hide
          CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Side comment, would you mind performing a quick review of these:

          http://integration.moodle.org/job/Precheck%20remote%20branch/822/artifact/work/smurf.html

          A bunch of them seem fixable, really, TIA!

          Once amended, I think this is pretty ready for integration. Would try to get any other integrator on it just to verify we are not missing anything obvious.

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Side comment, would you mind performing a quick review of these: http://integration.moodle.org/job/Precheck%20remote%20branch/822/artifact/work/smurf.html A bunch of them seem fixable, really, TIA! Once amended, I think this is pretty ready for integration. Would try to get any other integrator on it just to verify we are not missing anything obvious. Thanks!
          Hide
          Sam Hemelryk added a comment - - edited

          +1 from me.

          One thing to note, more in relation to the linked issue for conversion of MUC to this new system was that MUC presently holds no dependency on the database, and as such could be used before the database is available (presently DB is always used first). This would hold some bearing to MUC as the default lock is the currently used database and $DB is being used.
          Personally I don't think this is an issue, but am noting it just so that it is on the record.

          Show
          Sam Hemelryk added a comment - - edited +1 from me. One thing to note, more in relation to the linked issue for conversion of MUC to this new system was that MUC presently holds no dependency on the database, and as such could be used before the database is available (presently DB is always used first). This would hold some bearing to MUC as the default lock is the currently used database and $DB is being used. Personally I don't think this is an issue, but am noting it just so that it is on the record.
          Hide
          CiBoT added a comment -

          Results for MDL-25500

          • Remote repository: git://github.com/damyon/moodle.git
          Show
          CiBoT added a comment - Results for MDL-25500 Remote repository: git://github.com/damyon/moodle.git Remote branch MDL-25500 -master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/864 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/864/artifact/work/smurf.html
          Hide
          Damyon Wiese added a comment -

          Just pushed again to fix those last cime warnings.

          There is also a squashed branch "MDL-25500-master-squashed".

          Show
          Damyon Wiese added a comment - Just pushed again to fix those last cime warnings. There is also a squashed branch " MDL-25500 -master-squashed".
          Hide
          Marina Glancy added a comment -

          Hi, this is a great change.
          I can only suggest to add check for $CFG->preventfilelocking in file_lock_factory::is_available()
          but at the same time I don't see this is_available() function used much, for example it is not used in lock_config::get_lock_factory()

          the code looks very neat

          Show
          Marina Glancy added a comment - Hi, this is a great change. I can only suggest to add check for $CFG->preventfilelocking in file_lock_factory::is_available() but at the same time I don't see this is_available() function used much, for example it is not used in lock_config::get_lock_factory() the code looks very neat
          Hide
          Petr Škoda added a comment -

          the $CFG->preventfilelocking is meant for $CFG->dataroot only, that means it should not be used imo when $CFG->file_lock_root is explicitly specified. The is_available() should also probably verify the $CFG->file_lock_root actually exists

          Show
          Petr Škoda added a comment - the $CFG->preventfilelocking is meant for $CFG->dataroot only, that means it should not be used imo when $CFG->file_lock_root is explicitly specified. The is_available() should also probably verify the $CFG->file_lock_root actually exists
          Hide
          Damyon Wiese added a comment -

          OK - Thanks - I'll fix these tonight.

          Show
          Damyon Wiese added a comment - OK - Thanks - I'll fix these tonight.
          Hide
          Petr Škoda added a comment -

          Anyway I do not think this is a blocker - there are other things waiting for this (upgrade, cron) and it will be easy to fix later in master...

          Show
          Petr Škoda added a comment - Anyway I do not think this is a blocker - there are other things waiting for this (upgrade, cron) and it will be easy to fix later in master...
          Hide
          Damyon Wiese added a comment -

          Added one commit to the squashed branch to address these last comments - checks if preventfilelocking is set and file_lock_dir is not set, or file_lock_dir starts with the same path as dataroot.

          is_available() is then checked in lock_config so it will not use file_locking in this case.

          Show
          Damyon Wiese added a comment - Added one commit to the squashed branch to address these last comments - checks if preventfilelocking is set and file_lock_dir is not set, or file_lock_dir starts with the same path as dataroot. is_available() is then checked in lock_config so it will not use file_locking in this case.
          Hide
          Sam Hemelryk added a comment -

          Alrighty this has been integrated now.

          I made a commit on top of this work that makes a couple of very trivial changes to fix the following:

          • A couple of misc comment typo's
          • Added missing $CFG required global in file_lock_factory::is_available
          • Removed unused $CFG global from file_lock_factory::get_lock
          • A couple of trivial phpdoc tweaks
          • Removed unused $DB global from postgres_lock_factory::get_index_from_key
          • Added global namespace \ to exceptions in get_index_from_key (tested and found they didn't resolve)
          Show
          Sam Hemelryk added a comment - Alrighty this has been integrated now. I made a commit on top of this work that makes a couple of very trivial changes to fix the following: A couple of misc comment typo's Added missing $CFG required global in file_lock_factory::is_available Removed unused $CFG global from file_lock_factory::get_lock A couple of trivial phpdoc tweaks Removed unused $DB global from postgres_lock_factory::get_index_from_key Added global namespace \ to exceptions in get_index_from_key (tested and found they didn't resolve)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yay, hope testing does not lock this! ROFL

          Show
          Eloy Lafuente (stronk7) added a comment - Yay, hope testing does not lock this! ROFL
          Hide
          Damyon Wiese added a comment -

          Thanks for those extra fixes Sam.

          Show
          Damyon Wiese added a comment - Thanks for those extra fixes Sam.
          Hide
          Damyon Wiese added a comment -

          I just noticed the testing instructions need a little tweaking (no memcache tests).

          Show
          Damyon Wiese added a comment - I just noticed the testing instructions need a little tweaking (no memcache tests).
          Hide
          Damyon Wiese added a comment -

          Testing instructions fixed.

          Show
          Damyon Wiese added a comment - Testing instructions fixed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Just note that ci server (using mysql) is failing once this issue landed:

          http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/2351/

          the laptop ci server, running postgresql, is passing ok, apparently.

          Show
          Eloy Lafuente (stronk7) added a comment - Just note that ci server (using mysql) is failing once this issue landed: http://integration.moodle.org/job/20.%20Run%20phpunit%20UnitTests%20(master)/2351/ the laptop ci server, running postgresql, is passing ok, apparently.
          Hide
          Damyon Wiese added a comment -

          Thanks Eloy - noting that this is correctly detecting that MySQL locking is moronic - not sure why I didn't see this fail before - but the tests are correct and the MYSQL GET_LOCK cannot be used here unless we get a new DB connection for each lock (bleh).

          Show
          Damyon Wiese added a comment - Thanks Eloy - noting that this is correctly detecting that MySQL locking is moronic - not sure why I didn't see this fail before - but the tests are correct and the MYSQL GET_LOCK cannot be used here unless we get a new DB connection for each lock (bleh).
          Hide
          Damyon Wiese added a comment -

          Pushed a fix here:

          git pull git://github.com/damyon/moodle.git MDL-25500-master-fix1

          The fix is to delete the mysql_lock_factory (it will fallback to db_record_lock_factory).

          Using GET_LOCK here would break session locking because a thread can only have 1 open lock in MySQL. The only possible fix would be to open an new DB connection for every lock (which seems worse than just using db_record_lock_factory).

          Show
          Damyon Wiese added a comment - Pushed a fix here: git pull git://github.com/damyon/moodle.git MDL-25500 -master-fix1 The fix is to delete the mysql_lock_factory (it will fallback to db_record_lock_factory). Using GET_LOCK here would break session locking because a thread can only have 1 open lock in MySQL. The only possible fix would be to open an new DB connection for every lock (which seems worse than just using db_record_lock_factory).
          Hide
          Marina Glancy added a comment -

          fix pulled, thanks

          Show
          Marina Glancy added a comment - fix pulled, thanks
          Hide
          Andrew Nicols added a comment -

          Tests passing in MySQL and Postgres:

          Postgres:
          2012 im:master> mdk phpunit -r -u lib/tests/lock_test.php
          Initialising Moodle PHPUnit test environment...
          Moodle 2.7dev (Build: 20140123), pgsql, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /private/srv/www/loganberry.local/data/im/moodle/phpunit.xml
          
          .
          
          Time: 5.64 seconds, Memory: 53.50Mb
          
          OK (1 test, 19 assertions)
          2013 im:master> mdk phpunit -r -u lib/tests/lock_config_test.php
          Initialising Moodle PHPUnit test environment...
          Moodle 2.7dev (Build: 20140123), pgsql, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /private/srv/www/loganberry.local/data/im/moodle/phpunit.xml
          
          .
          
          Time: 1.85 seconds, Memory: 53.50Mb
          
          OK (1 test, 3 assertions)
          
          MySQL
          2015 im_my:master> mdk phpunit -r -u lib/tests/lock_test.php
          Initialising Moodle PHPUnit test environment...
          Moodle 2.7dev (Build: 20140123), mysqli, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /private/srv/www/loganberry.local/data/im_my/moodle/phpunit.xml
          
          .
          
          Time: 8.22 seconds, Memory: 60.00Mb
          
          OK (1 test, 18 assertions)
          2016 im_my:master> mdk phpunit -r -u lib/tests/lock_config_test.php
          Initialising Moodle PHPUnit test environment...
          Moodle 2.7dev (Build: 20140123), mysqli, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /private/srv/www/loganberry.local/data/im_my/moodle/phpunit.xml
          
          .
          
          Time: 1.6 seconds, Memory: 52.00Mb
          
          OK (1 test, 3 assertions)
          

          Running tests on MSSQL and Oracle now.

          Show
          Andrew Nicols added a comment - Tests passing in MySQL and Postgres: Postgres: 2012 im:master> mdk phpunit -r -u lib/tests/lock_test.php Initialising Moodle PHPUnit test environment... Moodle 2.7dev (Build: 20140123), pgsql, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from / private /srv/www/loganberry.local/data/im/moodle/phpunit.xml . Time: 5.64 seconds, Memory: 53.50Mb OK (1 test, 19 assertions) 2013 im:master> mdk phpunit -r -u lib/tests/lock_config_test.php Initialising Moodle PHPUnit test environment... Moodle 2.7dev (Build: 20140123), pgsql, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from / private /srv/www/loganberry.local/data/im/moodle/phpunit.xml . Time: 1.85 seconds, Memory: 53.50Mb OK (1 test, 3 assertions) MySQL 2015 im_my:master> mdk phpunit -r -u lib/tests/lock_test.php Initialising Moodle PHPUnit test environment... Moodle 2.7dev (Build: 20140123), mysqli, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from / private /srv/www/loganberry.local/data/im_my/moodle/phpunit.xml . Time: 8.22 seconds, Memory: 60.00Mb OK (1 test, 18 assertions) 2016 im_my:master> mdk phpunit -r -u lib/tests/lock_config_test.php Initialising Moodle PHPUnit test environment... Moodle 2.7dev (Build: 20140123), mysqli, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from / private /srv/www/loganberry.local/data/im_my/moodle/phpunit.xml . Time: 1.6 seconds, Memory: 52.00Mb OK (1 test, 3 assertions) Running tests on MSSQL and Oracle now.
          Hide
          Andrew Nicols added a comment -
          MSSQL:
          2012 im_ms:master> mdk phpunit -r -u lib/tests/lock_test.php
          Initialising Moodle PHPUnit test environment...
          PHPUnit ready!
          Moodle 2.7dev (Build: 20140123), mssql, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /home/nicols/Public/banana.local/data/im_ms/moodle/phpunit.xml
          
          .
          
          Time: 6.71 seconds, Memory: 48.75Mb
          
          OK (1 test, 18 assertions)
          2013 im_ms:master> mdk phpunit -r -u lib/tests/lock_config_test.php
          Initialising Moodle PHPUnit test environment...
          PHPUnit ready!
          Moodle 2.7dev (Build: 20140123), mssql, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /home/nicols/Public/banana.local/data/im_ms/moodle/phpunit.xml
          
          .
          
          Time: 513 ms, Memory: 49.00Mb
          
          OK (1 test, 3 assertions)
          
          Show
          Andrew Nicols added a comment - MSSQL: 2012 im_ms:master> mdk phpunit -r -u lib/tests/lock_test.php Initialising Moodle PHPUnit test environment... PHPUnit ready! Moodle 2.7dev (Build: 20140123), mssql, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from /home/nicols/Public/banana.local/data/im_ms/moodle/phpunit.xml . Time: 6.71 seconds, Memory: 48.75Mb OK (1 test, 18 assertions) 2013 im_ms:master> mdk phpunit -r -u lib/tests/lock_config_test.php Initialising Moodle PHPUnit test environment... PHPUnit ready! Moodle 2.7dev (Build: 20140123), mssql, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from /home/nicols/Public/banana.local/data/im_ms/moodle/phpunit.xml . Time: 513 ms, Memory: 49.00Mb OK (1 test, 3 assertions)
          Hide
          Andrew Nicols added a comment -
          Oracle:
          2024 im_o:master> mdk phpunit -r -u lib/tests/lock_test.php
          Initialising Moodle PHPUnit test environment...
          PHPUnit ready!
          Moodle 2.7dev (Build: 20140123), oci, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /home/nicols/Public/banana.local/data/im_o/moodle/phpunit.xml
          
          .
          
          Time: 21.08 seconds, Memory: 62.25Mb
          
          OK (1 test, 18 assertions)
          2025 im_o:master> mdk phpunit -r -u lib/tests/lock_config_test.php
          Initialising Moodle PHPUnit test environment...
          PHPUnit ready!
          Moodle 2.7dev (Build: 20140123), oci, 7c4044a54f11df17f619487e6629bcc97ee533e3
          PHPUnit 3.7.28 by Sebastian Bergmann.
          
          Configuration read from /home/nicols/Public/banana.local/data/im_o/moodle/phpunit.xml
          
          .
          
          Time: 7.05 seconds, Memory: 54.50Mb
          
          OK (1 test, 3 assertions)
          
          Show
          Andrew Nicols added a comment - Oracle: 2024 im_o:master> mdk phpunit -r -u lib/tests/lock_test.php Initialising Moodle PHPUnit test environment... PHPUnit ready! Moodle 2.7dev (Build: 20140123), oci, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from /home/nicols/Public/banana.local/data/im_o/moodle/phpunit.xml . Time: 21.08 seconds, Memory: 62.25Mb OK (1 test, 18 assertions) 2025 im_o:master> mdk phpunit -r -u lib/tests/lock_config_test.php Initialising Moodle PHPUnit test environment... PHPUnit ready! Moodle 2.7dev (Build: 20140123), oci, 7c4044a54f11df17f619487e6629bcc97ee533e3 PHPUnit 3.7.28 by Sebastian Bergmann. Configuration read from /home/nicols/Public/banana.local/data/im_o/moodle/phpunit.xml . Time: 7.05 seconds, Memory: 54.50Mb OK (1 test, 3 assertions)
          Hide
          Andrew Nicols added a comment -

          Passing this issue. I've performed the following:

          • All unit tests on:
            • MySQL
            • Postgres
            • MSSQL
            • Oracle

          I've also played with locktest.php on all of the database engines, using all of the indicated lock_factory settings.

          Thanks Damyon - all good

          Show
          Andrew Nicols added a comment - Passing this issue. I've performed the following: All unit tests on: MySQL Postgres MSSQL Oracle I've also played with locktest.php on all of the database engines, using all of the indicated lock_factory settings. Thanks Damyon - all good
          Hide
          Marina Glancy added a comment -

          Thanks for your hard work. Your code has now become a part of Moodle!

          Show
          Marina Glancy added a comment - Thanks for your hard work. Your code has now become a part of Moodle!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              16 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: