Modern event based lock framework proposal

Just in case, did you look at http://drupal.org/project/redis module? I did moved all the lock business as a lock backend interface (I needed it at this point). For the usage I don't have a factory/accessor to fetch the right instance back, but a temporary procedural wrapper arround because it's a project that aims to be usable on D7, but basically it's exactly what you are trying to do here.

+++ b/includes/bootstrap.inc
@@ -2919,6 +2915,72 @@ function drupal_placeholder($text) {
+ * Factory function for the locking framework.
+ * In most environments, multiple Drupal page requests (a.k.a. threads or ¶

Trailing whitespace. Also there should be a blank line in between these two lines.

+++ b/includes/bootstrap.inc
@@ -2919,6 +2915,72 @@ function drupal_placeholder($text) {
+    $class_name = variable_get('lock_class', 'LockDatabase');

I think in the cache system, we also support a "cache_$bin_class" or something like that. Here we could have a "lock_$name_class" or something. Is that left out intentionally?

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+   * Release a lock previously acquired by lock_acquire().

This should be lock->acquire().

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+  ¶
...
+  protected $locks; ¶

Trailing whitespace.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
-  global $locks;
...
+          // We track all acquired locks in the global variable.

Yay!
The comment is outdated now.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+    $expire = microtime(TRUE) + $timeout;

I know this was in the code before, but a comment why we do not use REQUEST_TIME would be good I think.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+      // We always want to do this code at least once.

Again already there, but "do the code" should be "run the code" or "execute the code" or something.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+          // Suppress the error. If this is our first pass through the loop,
+          // then $retry is FALSE. In this case, the insert must have failed
+          // meaning some other request acquired the lock but did not release it.

Again can be ignored, because it is already there, but it is unclear to me, how the insert can fail, but the lock is not in $this->locks.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+          // We decide whether to retry by checking lock_may_be_available()

That should be lock()->mayBeAvailable()

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+          // Since this will break the lock in case it is expired.

Since -> since

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+        // We only retry in case the first attempt failed, but we then broke
+        // an expired lock.

This wraps early.

+++ b/includes/lock.inc
@@ -1,271 +1,209 @@
+        ->condition('value', $lock['value'])

That should be $this->id.

If any of the changes that were in the code already are non-trivial, I'll happily open a new issue, but I thought I'd post it anyway.

Fixed the coding standards, trivial fixes. Some of the comments above still stand, but are all already existant in the current code.

Here's a first stab at implementing #2.
I agree this feels much more natural.

You actually ignored my #3 comment?

EDIT: Using an object for the lock itself would probably imply a strong coupling between the lock instance and the lock backend itself: it seems less pluggable IMHO. A lock is and will remain only a name, a token, no more: I see why using a class seems nice (the lock as an object) but it also complexify the design here.

I'm generally not certified to judge OO design patterns, so this might be totally off, but:
I think the coupling only starts when consumers of the Lock API use backend-specific properties of the lock object; which we explicitly tell people not to do. If people stick with the API there's no coupling at all.

Regarding the patch: I have a cleaned-up version locally (with less syntax errors...), but I can't get lock tests to pass, so I didn't post it yet.

Yes but I what I meant is that by using a class for lock itself, you are hiding the backend behind: this means that people who write a backend must rewrite the lock class itself, not a simpler backend interface.

Plus, at runtime, there will be some shared properties among all locks (such as known lock names actually being used or such): you have to share these properties at backend class level, not lock class level (else you would be forced to introduce a global state, i.e. static properties which you can avoid by moving them at the backend level): so you have to derivate the whole design to a backend interface, plus various backend implementations, plus lock class, plus potentially (but not mandatory) introduce this new lock class.

This is not bad, I mean it's OK, but does this really serve any purpose to add the lock class?

You code should not work because in your patch the lock() static accessor does not uses any parameter while you are using it like this: $lock = lock('cron');. So you are indeed using the backend directly, but as is you were using the lock as a single instance.

Something more likely to work could be:

namespace Drupal\Lock;

interface Drupal\Lock\LockBackendInterface {
  public function acquire($name);
  public function release($name);
  // ...
}

class Drupal\Lock\DatabaseLockBackend {
  // Real implementation
}

Then static registry and usage would be:

public function lock_get_backend() {
  static $instance;
  // Some code to handle a singleton
  return $instance;
}

$lockBackend = lock_get_backend();
if ($lockBackend->acquire('some_name')) {
  // Do your heavy stuff
  $lockBackend->release('some_name');
} else {
  // Technical error: could not acquire the lock.
}

Now, if you really want to implement a lock class, then you probably should add this to the first piece of pseudo code upper:

namespace Drupal\Lock;

class Drupal\Lock\Lock {
  protected $backend;
  protected $name;

  public function __construct(LockBackendInterface $backend, $name) {
    $this->backend = $backend;
    $this->name = $name;
  }

  public function acquire() {
    return $this->backend->acquire($this->name);
  }

  public function release() {
    return $this->backend->release($this->name);
  }
}

Then, factory and usage would become this:

use Drupal\Lock\Lock as Lock;

public function lock_get_lock($name) {
  static $backendInstance;
  // Some code to handle a singleton
  return new Lock($backendInstance, $name);
}

$lock = lock_get_lock();
if ($lock->acquire()) {
  // Do your heavy stuff
  $lock->release();
} else {
  // Technical error: could not acquire the lock.
}

Which basically would be the same, with the exception that you introduce a proxy pattern here, lock instance by itself does not serve any purpose here: I don't see where either the framework or pure DX really benefit from this.

PS: Yes I'm coding PHP 5.3, seems more natural to push this kind of refactor directly in the right naming convention.

Re PS: By the way, the PHP code interpreter removes my backslashes \ which makes the PHP 5.3 code somewhat non readable.

Yes, I know the patch failed to update the procedural lock() implementation. I'll post an updated one soon, even though it fails tests, so you can see where this is headed. The gist is very similar to what you proposed last:

$lock = lock('mylockname');
if ($lock->acquire()) {
  $lock()->release();
}
else {
  // ...
}

Yup I understood that, what I was saying is maybe having the lock itself being an object doesn't really serve the purpose. Maybe it does, maybe it doesn't; But further than that, in your actual latest patch, the design does not make clear if the lock class is the lock backend or the lock object itself. Remember that what's need to be pluggable, and really easy to write is the backend, so if you're going to implement the lock itself as a class, you need to do a really good design that decouples it from the backend so that the backend developers won't have to worry about: your design does not really clear that question.

I'd see something more like this.

Can someone queue the patch for testing please? forgot to set "Needs review".

I don't see where saving you from using the name is really a benefit?

EDIT: I mean, it's handy to write, but that's pretty much it.

I think that in term of design it's probably better to keep only one backend instance behind, it allows it to share stuff like, in the legacy lock.inc, the $locks global: here it will be come a property of the backend removing the global state for this property.

If the lock should be an instance to you, it shouldn't be an instance of the backend itself.

Re-EDIT: To go further, there are some methods in the lock.inc file, such as the lock_release_all() function that is not owned by the lock itself, but by the backend: it is a nonsense to have one backend instance per lock doing global operations over it.

Re-re-EDIT: If you want to keep lock itself as an isntance, you have to keep a lock instances registry in sync, because when you call lock release all over the backend, all these instances must be invalidated directly.

My opinion is that using a single lock instance introduce a lot of code indirection that is unnecessary.

EDIT again: Believe me I'm trying to write the code the same time, it's not trivial: using the backend directly as in my first patch is really easier to write and more maintainable: I did my first patch in something like 15mn tops; Honestly no wonder why the original issue author is really slow to write the "lock as class" way: it's a lot more complex.

I have an intermediate solution, way easier to implement:

1. Call acquire() over the backend, and return a LockToken instance (or FALSE or throw an exception if fail).
2. This LockToken instance has a magic __destruct() method implementation that will release the lock gracefully when going out of scope. Just as the future contextes and the database transactions.
3. Additional set of method such as disableAutoRelease() and release() over the token, but that's it, no other stuff to do.

Pros:
1. No need of the name parameter except for acquire() and wait().
2. You can have fine control over the release mecanism.
3. No need to keep a registry in sync: a token will die eventually whatever happens.

Cons:
1. If the same lock is taken deeper in the call stack, the new tokens can accidentally release the first one (they will go out of scope before the first token): I see two potential solutions:
1a. At lock update force the disableAutoRelease() of new instances.
1b. At lock acquire, keep a token registry and serve the same instance twice: once again the problem remains almost the same as using lock instances in term of sync (sync is always a lot of complexity).
2. The backend implements must take into account this token because the backend itself will create them: it adds complexity for the backend developers (we have a risk to see more errenous backends this way).
3. There is no way to keep backward compatiblity with old procedural functions, may be except by forcing disableAutoRealease() in lock_acquire() procedural function.

Using this design, we can move the token registry complexity outside the backend itself by using a proxy pattern arround the real backend: a common lock factory that will embed the backend and keep the sync itself.

IMHO it's the same problem than the cache backends: by adding complexity to the whole design you may confuse backend developers.

An example of the patch with the token method (but no registry) as a PoC.

Just for fun, updated patch.

index e69de29..0944d10 100644
+++ b/includes/Drupal/Lock/DatabaseLockBackend.phpundefined

+++ b/includes/Drupal/Lock/DatabaseLockBackend.phpundefined
@@ -0,0 +1,97 @@

@@ -0,0 +1,97 @@
+<?php
+
+namespace Drupal\Lock;
+

This should have a @file docblock.

+++ b/includes/Drupal/Lock/DatabaseLockBackend.phpundefined
@@ -0,0 +1,97 @@
+
+class DatabaseLockBackend extends LockBackendAbstract {

Most classes/methods in the patch are missing docblocks. I know many classes in core don't have either but we agreed on adding a Implements/Overrides ... docblock similar to hooks für class methods a while ago (check coding standards) so let's try to follow that at least for now stuff :)

Also, at least the classes need a short docblock anyway.

+++ b/includes/Drupal/Lock/DatabaseLockBackend.phpundefined
index e69de29..9269f20 100644
+++ b/includes/Drupal/Lock/LockBackendAbstract.phpundefined

+++ b/includes/Drupal/Lock/LockBackendAbstract.phpundefined
+++ b/includes/Drupal/Lock/LockBackendAbstract.phpundefined
@@ -0,0 +1,64 @@

@@ -0,0 +1,64 @@
+<?php
+

Also missing a @file docblock.

+++ b/includes/Drupal/Lock/LockBackendAbstract.phpundefined
@@ -0,0 +1,64 @@
+  /**
+   * Current page lock token identifier.
+   * ¶
+   * @var string
+   */
+  protected $lockId;
+
+  /**
+   * Existing locks for this page.
+   * ¶
+   * @var array

Some trailing spaces.

The docblocks in the interfaces below have them too.

+++ b/includes/Drupal/Lock/LockBackendInterface.phpundefined
@@ -0,0 +1,60 @@
+   * @param float $timeout = 30.0
+   *   (optional) Lock lifetime in seconds.

Never seen the default value documented in a docblock like this, is this really valid and properly recognised by our and other parsers (e.g. those of IDE's like Netbeans)?

+++ b/includes/Drupal/Lock/LockBackendInterface.phpundefined
@@ -0,0 +1,60 @@
+   * Get the unique page token for locks. Locks will be wipeout at each end of
+   * page request on a token basis.
+   * ¶

Second sentence should be on a separate line (with an empty line in between) as it's not part of the one-sentence short description of the method.

+++ b/includes/lock.incundefined
@@ -61,31 +61,43 @@
+// FIXME: Temporary manual file inclusion, since no autoloader still has been

I know his is just a tempory comment, but it should be a @todo and also proper english (..still has been committed yet...) :)

Actually, I think this *has* been commited in the meantine, see http://api.drupal.org/api/drupal/core--includes--bootstrap.inc/function/...

Powered by Dreditor.

I guess the @file docblock should disappear when writing PSR-0 code, since it's one class per file. The @file docblock had only one purpose, describe what's inside the file: if the file contains only one class, it's self-describing so @file is a unneeded redundancy.

The missing docblocks are only implementing documentation, I agree that everything needs to be documenting, but once again OOP code self-documents itself when it comes to interface implementation. Following multiple refactoring with docblocks is a nightmare because each time you move a method definition inside the hierarchy you have tens of dockblocks to fix when they are not really needed.

FIXME and @todo both means something different. FIXME means "fix me" I guess that's exactly what I wanted to say here:)

Thanks for the review, will fix the patch.

Did not fix everything, but a lot of stuff. Should pass (I guess).

0 passes.

That's weird, test bot says that status is PASS. FYI using this patch on a D8 git actually works flawlessly when using it.

EDIT: Saw that there is a lot of SQL exceptions in test log, I should check a bit deeper.
Re-EDIT: Running some tests I got what seems to be arbitrary SQL errors, I guess that something in my changes make that happen. I will continue working on it using the locking framework unit tests as reference.

The testbot says pass because there are no exceptions/failures detected, which is often when the testbot crashes completely. In these cases, have a detailed look at the review log, where you can see:

exception 'PDOException' with message 'SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '))
ORDER BY fit DESC
LIMIT 1 OFFSET 0' at line 1' in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc:2132
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc(2132): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc(664): DatabaseStatementBase->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/select.inc(1284): DatabaseConnection->query('SELECT menu_rou...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/menu.inc(3269): SelectQuery->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/menu.inc(3080): _menu_find_router_path('admin/structure...')
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/menu/menu.module(198): menu_link_save(Array)
#6 [internal function]: menu_enable()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(795): call_user_func_array('menu_enable', Array)
#8 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/module.inc(458): module_invoke('menu', 'enable')
#9 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(1312): module_enable(Array, false)
#10 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/aggregator/aggregator.test(10): DrupalWebTestCase->setUp('aggregator', 'aggregator_test')
#11 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/aggregator/aggregator.test(856): AggregatorTestCase->setUp()
#12 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(476): FeedParserTestCase->setUp()
#13 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(367): DrupalTestCase->run()
#14 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('1', 'FeedParserTestC...')
#15 {main}FATAL AJAXFrameworkTestCase: test runner returned a non-zero error code (1).
....

exception 'PDOException' with message 'SQLSTATE[HY000]: General error: 1114 The table 'simpletest' is full' in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc:2132
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc(2132): PDOStatement->execute(Array)
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/database.inc(664): DatabaseStatementBase->execute(Array, Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/database/mysql/query.inc(36): DatabaseConnection->query('INSERT INTO {si...', Array, Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(186): InsertQuery_mysql->execute()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/drupal_web_test_case.php(475): DrupalTestCase::insertAssert('1', 'XMLRPCBasicTest...', false, 'The test did no...', 'Completion chec...', Array)
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(367): DrupalTestCase->run()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('1', 'XMLRPCBasicTest...')
#7 {main}

.... And finally:

PDOException: SQLSTATE[HY000]: General error: 2006 MySQL server has gone away: SELECT last_prefix FROM {simpletest_test_id} WHERE test_id = :test_id LIMIT 0, 1; Array
(
[:test_id] => 1
)
in simpletest_last_test_get() (line 237 of /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/simpletest.module).

The second exception sounds like an issue with the testbot (disk full?), not sure about the first.

Yes it is, can someone requeue the patch for testing?

#25: lock-oop-interface-with-token-3.patch queued for re-testing.

Yes, and I really don't know why...

Getting back to work on this patch in the next few days!

Chaging title to a more accurate one.

I did some minor typo fixes, I am currently running the full latest 8.x (with the patch applied) test suite over my own development box. I hope it will be easier to find out the problem running it locally.

Running with sudo -u www-data php -f core/scripts/run-tests.sh -- --php `which php` --url http://d8-git.blaster --verbose --color --all since a few minutes, still no crashes.

Ok, fun stuff, running the long run with --all always end up failing at the same place, but when running the crashing tests standalone (using --file or --class) it passes. Any ideas?

Could you maybe upload as a patch so we can view the tesbot's results?

I am tired. You are testing the patch in #25, plus typo fixes.

@webchick Yes I am! The only typo fix really is escaping correctly a class name in a string (which didn't seem to affect the code anyway).

Delete comment (wrong issue).

Commenting on the design (the test failures seem to indicate that the current patch doesn't release the locks properly, so one test every two fails):

• lock_get_factory() is a factory of a factory, which doesn't make much sense
• LockFactory does nothing except proxying the backend, and as a consequence should be removed
• LockToken release the lock but doesn't acquire it. It should acquire it in __construct() instead of relying on the lock being already acquired
• DatabaseLockBackend relies on the DatabaseConnection object but doesn't hold it, as a consequence the destruct order that should be LockToken > DatabaseLockBackend > DatabaseConnection might not be respected, and DatabaseLockBackend may end up trying to release a lock after the database connection has been destroyed during the PHP shutdown sequence.
• Also, because we have both a shutdown function and a destructor in LockToken, locks might end up being released several times

LockFactory does nothing except proxying the backend, and as a consequence should be removed

Partially agree: I introduced the factory so the LockToken class would be not handled by the backend: this removes a part of the complexity for backend developers. The "Factory" name is wrong, but the overall design is OK.

LockToken release the lock but doesn't acquire it. It should acquire it in __construct() instead of relying on the lock being already acquired

That, I do not agree: the token exists only if the lock was acquired, so we cannot acquire it into its constructor. I know that releasing an acquired lock somewhere else that where you created it seems wrong, but I don't see any other choice here.

DatabaseLockBackend relies on the DatabaseConnection object but doesn't hold it, as a consequence the destruct order that should be LockToken > DatabaseLockBackend > DatabaseConnection might not be respected, and DatabaseLockBackend may end up trying to release a lock after the database connection has been destroyed during the PHP shutdown sequence.

True, same problem everywhere in Drupal right now. It might be changed to hold a pointer then, but it's unelegant somehow. Any better suggestion is welcome (for what it worth, the actual stable API as you wrote it originally for D6.16 is subject to the same flaw since D7 uses an object for database connection, it never has been fixed before).

Also, because we have both a shutdown function and a destructor in LockToken, locks might end up being released several times

The shutdown handler is only a failsafe in case some locks may not have been released. There is two use cases thought:

• LockToken is in auto release mode: it will always be released before the shutdown handler call. If not, that means it has been kept as a reference somewhere: it's not designed to be used this way, and it is a real bug to be fixed at the source, not in the lock API
• LockToken is not in auto release mode: if we forget to release it, the lock will be kept somewhere, but the token will probably have been destructed before: the shudown handler will work as expected. If the token has been kept as a reference somewhere or not, no side effect since the object won't trigger the release itself

Whatever it goes through, releasing twice a lock will not generate errors, it's a simple delete, in worst case it the SQL will be run twice with no side effect. Maybe the release() method over the backend could be documented as silent if we try to release a non existing lock.

EDIT: I actually see other design flaws, but not the one you quoted. I will do a post about it as soon as tests passes.

the test failures seem to indicate that the current patch doesn't release the locks properly, so one test every two fails

Thanks, I will investigate this, I was just running the lock functional tests.

Partially agree: I introduced the factory so the LockToken class would be not handled by the backend: this removes a part of the complexity for backend developers. The "Factory" name is wrong, but the overall design is OK.

It removes absolutely no complexity, has zero added value and should be removed... and it has mainly no added value because the LockToken should handle acquiring the lock:

That, I do not agree: the token exists only if the lock was acquired, so we cannot acquire it into its constructor. I know that releasing an acquired lock somewhere else that where you created it seems wrong, but I don't see any other choice here.

No, the job of the token is to wrap a lock, acquiring the lock in the constructor makes total sense here. If acquiring the lock fails for some reason, we can always trigger an exception inside the constructor (which is perfectly ok).

It removes absolutely no complexity, has zero added value and should be removed... and it has mainly no added value because the LockToken should handle acquiring the lock:

It actually keeps the backend token agnostic, while keeping the procedural accessors being accessors only: the procedural accessors can then be removed as the code is ported to use the new API until it remains only one getter, pretty much as the cache() one.
So, for backend developers, it's easier because the backend is really encapsulated to the strict minima. For the user it's only a proxy he is not aware of, and expose a higher level API without the lockMayBeAvaiable() etc...

It's still missing some glues pieces to definitely get rid of the lockMayBeAvailable() and wait(), I could see something like this:

lock()->doWhenAcquired('some_lock_name', function() {
  // Do something.
});

Which is quite elegant; because we can remove the token usage out of the user's hand (and the wait() and other methods as well). In case the lock never gets free after some wait iterations, we could even throw some LockCantBeAcquiredException.

So this, plus the direct lock()->acquire() method, returning a LockToken but throwing an exception in case it could not, seems like a killer API allowing to do everything we need or already do actually.

Considering all of this: we have wait() and maybeavaialble() implicit control by the Factory with only two methods, one strict (throwing exception in case no acquire) and the other more flexible (will iterate until it gets it) --that can change of name-- but we have a minimal encapsulated backend: it's a win-win API.

No, the job of the token is to wrap a lock, acquiring the lock in the constructor makes total sense here. If acquiring the lock fails for some reason, we can always trigger an exception inside the constructor (which is perfectly ok).

Throwing exceptions inside a constructor might be safe in PHP (it is not in many languages), it still remain an ugly pattern. We still could if we implement my upper proposal: the exception seems natural here.

For the user it's only a proxy he is not aware of, and expose a higher level API without the lockMayBeAvailable() etc...

Those are not API functions and should be protected. If the function of the proxy is just to hide protected methods, it should definitely go away :)

Other then that, compare:

lock()->doWhenAcquired('some_lock_name', function() {
  // Do something.
});

with:

lock('some_lock_name')->doWhenAcquired(function() {
 // Do something.
});

... it's exactly the same thing, except that the second one is more natural because the lock() factory *actually returns a lock* (or in that case, a lock token, but that's exactly the same thing).

Ok, so new API proposal @all

class LockCannotBeAcquiredException extends \RuntimeException {};

class LockFactory
{
    /**
     * Acquire a lock.
     *
     * @param string $name
     *
     * @return LockToken
     *
     * @throws LockCannotBeAcquiredException
     *   If lock could not be acquired on first attempt.
     */
    public function acquire($name)
    {
        // ... Existing code.
    }

    /*
     * Acquire a lock and execute callback for the time being acquired.
     * The lock will be acquired in a safe way, calling multiple time wait()
     * and lockMayBeAvailable() until it acquires it.
     * 
     * Lock will be acquired and released implicetely, you don't need to
     * to do it manually.
     *
     * @param string $name
     * @param callback $callback
     *   Callback that will be run once the lock is acquired.
     *
     * @throws LockCannotBeAcquiredException
     *   In case something really bad happened (lock has not been acquired).
     */
    public function doOnceAcquired($name, $callback)
    {
        // ...
    }
}

Which gives us a usage as this:

try {
  $token = lock()->acquire('my_business_lock');
  my_business_rebuild();
  // Here token release the lock gracefully.
}
catch (LockCannotBeAcquiredException $e) {
  // Do whatever we can and hope this will work the next hit.
}

Or, in case we really need what we are rebuilding (menu and variable are critical for example):

if (menu_needs_rebuild()) {
  lock()->doOnceAcquired('menu_rebuild', '_menu_rebuild');
  // Let exceptions be thrown, we cannot proceed without a menu, this is fatal.
}

Throwing exceptions inside a constructor might be safe in PHP (it is not in many languages), it still remain an ugly pattern. We still could if we implement my upper proposal: the exception seems natural here.

Do you have pointers to back this up? As far as I know, it's pretty much the opposite: throwing exceptions inside a constructor is pretty much only unsafe in C++. At least in Java, C# and Javascript it is safe and recommended.

@#46 The second accessor clearly will take care of some business stuff, while the first one is clearly just a proxy method to fetch an object.

@#48 Yes, may be in Java, but I higly trust the Java VM and C#/.Net VM. PHP has its moment and sometime really weird behaviors I don't trust, although you might be right/ Anyway, this does not changes anything, this is the least of the details about those changes.

Those are not API functions and should be protected. If the function of the proxy is just to hide protected methods, it should definitely go away :)

You are misreading what I say: the backend are all public methods so that we can do whatever we actually do right now; But they are no public methods in the sense that user should never use them manually: we need a layer between the user accessor and the backend to take care of this complexity: it's separation of concerns: the factory here is the separation layer which ties all together; let me do another patch and you will see that this layer can be pretty thin, expose a deeply simple API; While keeping an accessor onto the backend for insane developers to be able to do whatever they want to do with the backend directly (although not advised, but I like to let some doors opened, if we don't developers will find another one anyway).

Good news, failures where definitely a typo error, unset'ing $this->lock[$name] instead of $this->locks[$name].

Re-testing on my side and making a new patch then.

Here is a new version of the patch:

• Added the two API methods as described upper (using a callback, or throwing exceptions when cannot acquire).
• Fixed the backward compatibility functions, one was dropping the timeout parameter: it should fix actual core tests.
• Also fixed a typo error deep inside the code, should fix all the other exceptions we had with PDO: this was due to non released lock leading to some cache mis-built.
• I fixed some doxygens to be more precise, and removed all comments about the lock.inc replacement, which should not happen anymore now.
• Added the shutdown handler in a more elegant fashion: uses an object method as callback instead of a global scope procedural function that serves only this purpose.

Oups, fixed version.

Now, the PoC works, we can talk features and design.

As said by Damien, there are some details that do not fit very well, where he and I disagree is I think it's mostly about naming, but discussion is definitely opened about the design too.

I kinda like the callback approach for using the locking layer, what's missing out there is the timeout parameter. Is that really revelant to keep a timeout parameter anyway, since the wait() method is supposed to be the most reactive as it can?

The callback method allows us two things: more natural writing/reading of locked operations, and for the sake of code reusability it also allows us to port really easily what core already does as locked: for the menu rebuild for example, everything's already into functions.

I almost had removed the lockMayBeAvailable() method at all, it doesn't mean anything from an API point of view: what's important is the wait() method. Some backends would allow to implement it in a non active sleep fashion (blocking but not CPU/SQL intensive) which would be better. I kept the method anyway because it was handy to write some tests, but I'm convinced it doesn't serve any purpose for users.

Join with original post

It would allow using more than one lock backend on the same site. This might be useful if you wanted to use a different class to prevent stampedes vs. race conditions.

I think this is a really bad idea: code that uses the lock API don't know which backend it uses. We also risk to stall locks, or even worse, taking the same lock on two different backends, thus nullifying the use of it. It's good for making it more easily swappable thought.

We'd be able to apply the (not yet committed) patterns in #1164484: Cache backend consistency runtime check and graceful downgrade mecanism for graceful fallback and #1167144: Replace hard coded database schema for cache bins with hook_cache_bin_info() to remove dependencies on system module.

For that, I definitely agree, we need a common pattern for those. I participated to the cache backend consistency issue development, but I think in the end that if WSCCI ends up with a good early plugin mecanisms, we may want to move this responsability over this generic API.

Currently whenever we change bootstrap order or add or remove locking to a low level bootstrap function, we have to move the require_once lock.inc and lock_initialize() code around

Not a problem anymore with my patch, everything's lazy loaded, and there is no more initialization phase than instanciating two objects using a variable: quite straightforward, it's now closer from the cache system it ever has been.

Having an interface enforces that backends comply to the API

Yay \o/ Definitely what we want! And in my code I tried to make the backend the most simple interface it can be. I surely hope to remove the lockMayBeAvailable() method too to reduce it a little but more, but one is not too many.
I chose to add an extra layer between the API the user will use and the backend to ensure backend does not have to implement the high level business at all and remain a set of primitive easily implementable with all backends. @catch, if you think a backend such as Memcache may have troubles to implement this, we have to discuss technical aspect of the backend interface. And we can without breaking the user API thanks to this additional layer, which is really a good thing.

If we want to write unit tests for systems that depend on the lock framework, this would make it possible to mock the lock API.

Right now, I did put everything in the lock functional tests class, but my goal is to move the tests I added into a unit test instead (no full Drupal bootstrapped) with one abstract getBackend() so that any backend provider could use the "official test suite" in order to test his backend.

Work to be done (or already done)

adds a Lock interface and LockDatabase default implementation to lock.inc

Not exactly into lock.inc, I did all that, but as PHP 5.3 namespaced code. But it's there. I also put a bonus NullLockBackend implementation, potentially for boosting up the install process.

instead of bootstrapping the lock system, bootstrap.inc just includes a factory function lock() - this handles loading the code and instantiates the class (defined in variables) into a static cache. This means there is no more need to explicitly load the code and initialize the framework - this should be a small memory/cpu saving on every request that doesn't try to acquire locks (nearly all of them).

Already done!

the external API for calling code changes from lock_acquire(), lock_wait() etc. to lock()->acquire(), lock()->wait(). That's it.

Already done too, hum, at least kept the backward compatibility, but pretty much as cache, we should remove it too once we agreed on a public API.

instead of registering a shutdown function for releasing locks at the end of the request, it uses __destruct()

For that last one, I'm sorry I probably won't agree: using the object destructor could be done, it should work since PHP properly destruct all objects at shutdown, but really, I'm no eager to experience possible PHP bugs we can see in this kind of edge cases: moreover, the actual database backend relies on DbTng, and we need it to be straight up while releasing all page locks: current page footer shutdown emulation is perfect because we are sure DbTng has not been destructed: it's safe.

As catch said, such API would need an edge case usage: stampede protection. The basics would be to allow, in case the lock could not be acquired, to execute the callback whatever happens, or propose an alternative result instead.

The timeout for those would also need to be configurable. For what it worth, I'm against letting the timeout configurable, I'd more go for a model where a default configurable timeout could be set for stampede protection, and this could be passed to the factory (need another name, maybe "manage"?) instance when it's created.

As a first glance, cold start, I could extend my patch by something like this:

  /**
   * Acquire the given lock then run the given callback, with a minimal wait
   * time for stampede protection.
   *
   * If the lock could not be acquired first time, successive retries will be
   * done until it is acquired. If it fails, the callback will be run no matter we
   * got the lock or not.
   *
   * @param string $name
   * @param callback $callback
   * @param array $args
   *   Optional parameters to pass to the callback.
   * @param float $timeout
   *   Maximum wait time, in seconds
   * @param callback $fail_callback
   *   Optional callback to run instead of the success callback if the lock failed
   *   to be acquired in time.
   * @param array $fail_args
   *   Optional parameters to pass to the failure callback.
   *
   * @return mixed
   *   Whatever the run callback did return.
   */
  public function doWithStampedeProtection($name, $callback, array $args = NULL, $timeout = 1.0, $fail_callback = NULL, $fail_args = NULL);

But this doesn't seem too good for DX. I'm waiting for catch suggestions to see what usage is done in the memcache module and in some core pieces.

Nother API proposal would be something like:

// Basic lock acquire, throws exception if cannot acquire it.
// The object you manipulate here is a really simple implementation.
// Returns whatever the callback returned.
$ret = lock()
  ->acquire('some_lock')
  // Lock timeout duration
  ->setTimeout(1.0)
  ->run(function () {
    // Some random operation to do
  })
  ->execute();

// Stampede protection oriented lock.
// The object you would manipulate here would be a different one.
// Return whatever the run callback runs (can be the fail result).
lock()
  ->acquireWithStampede('some_lock')
  // Lock timeout duration
  ->setTimeout(1.0)
  // Maximum wait() time (stampede protection)
  ->setDelay(1.0)
  ->run(function () {
    // Some random operation to do
  })
  ->fail(function ($some_parameter) {
    // Do something else, that returns a revelant value for us
  }, array($some_parameter))
  ->execute();

// Basic usage would remain, for most basic usages.
lock()->doWhenAcquired('some_lock', function () {
  // So something.
});

// And of course the equivalent for stampede...

// And yet the backend still accessible from here.
$backend = lock()->getBackend();
if ($backend->acquire('some_lock') || ($backend->wait('some_lock') && $backend->acquire('some_lock'))) {
  // Do something
}
else {
  // It failed.
}

// As other possible usages

// Acquire a lock, throw an exception if it cannot.
$token = lock()->acquire('some_lock');
// Do something
$token->release(); // Optional if in a block, with autorelease on destruct.

// Acquire a lock with retry, and a timeout, throw an exception if it cannot.
$token = lock()->acquire('some_lock', TRUE, 30.0);
// Same as upper.

// Etc etc etc.

Notice that this might sound juicy and stuff, but this would also be really slower, and for some usages, such as the cache get stampede protection, it may have a bad impact over performances.

Here is a working implementation of what I proposed with more consistent names and fully documented as doxygen. For fun!

I haven't looked at the code here in detail, but skimming the issue and seeing the doWhenAcquired() method, it reminds me of the Java synchronize keyword. That is a keyword on a method that handles locking by saying "only one thread can be in this method at once". The developer doesn't need to worry about lock names, just saying "this thing here? Don't do it more than once, k?"

Perhaps that's something we can emulate with closures here? (Just thinking aloud. "No" may well be the answer.)

Almost revelant comment :) Kidding, it is. No it doesn't "emulate closures", in fact it's more near from an ASYNC Future runner than the Java "synchronized" keyword. The LockRunner object has only one goal: running a long run operation, except that instead of running it ASYNC (the real Future) it runs it only once it acquired a lock and release the lock once finished. But it's PHP, it doesn't do threading, so it's still is blocking.

Yes it looks like "synchronized" it is true, but it's not a keyword, but a runner implementing it (almost like some other languages SDK will abstract threads).

Two of the parameters you can give to the runner are two closures, that it will run on success, and and optionally a closure that will run on failure; In fact not only closures, a pure PHP "callable" (a Closure, an object implementing __invoke, a function name, a create_function() return, or an array containing as values an object and a method name).

The goal is to provide an applicative lock; This is not exactly the same as "synchronized" which provides a lock at the object level and not an applicative mutex.

This API doesn't bring much more as the actual stable locking API, but it does bring exactly this:

• A high level API that hides the backend primitives
• A decoupling between the lock backend, and the high level primitive (keeping then the layer stable even if the backend API changes
• Most people that uses lock doesn't really how to use them, even in core they are 20 different ways of using them implemented: this forces the user to think of lock as a high level feature so he doesn't worry about the backend primitive
• Whatever complicated inside lies, the user just now he can run some function taking a mutex, and catch an exception if it fails, without worrying about technical details
• Backend primitive have been reduced a little more than in actual API: easier to implement, easier to test, highly injectable

The goal for developers is to replace this approximative code:

function some_operation() {
  if (lock_acquire('some_lock')) {
    return something_really_complicated_and_long_to_do();
  }
  else {
    if (lock_wait('some_lock') && lock_may_be_available('some_lock')) {
      return some_operation();
    }
    else {
      // OMG WTF DO I DO?!!!
     return some_other_failiure_operation();
    }
  }
}
$ret = some_operation();

By this straightforward and backend agnostic code:

$ret = lock()->doWhenAcquired('some_lock', function() {
  return something_really_complicated_and_long_to_do();
});

Or

$ret = lock()
  ->getRunner('some_lock')
  ->onSuccess('something_really_complicated_and_long_to_do')
  ->onFail('some_other_failiure_operation') // OMG WTF DO I DO?!!!
  ->execute();

Or if you don't want to write global functions:

$ret = lock()
  ->getRunner('some_lock')
  ->onSuccess(function () {
    return something_really_complicated_and_long_to_do();
  })
  ->onFail(function () {
    // OMG WTF DO I DO?!!!
    return some_other_failiure_operation();
  })
  ->execute();

As Crell told on IRC, this proposal has any sense only if we do not manage to find an already existing PHP API implementing this functionallity that fits with the core.

I did some research (really quick) and I was not able to find anything useful. Anyone that comes here and is interested by the topic is welcome to do such research and see if there's anything that could be useful.

The proposal here is a *proposal*, even if the patch looks really official, passes tests, is almost fully documented etc... it remains only a PoC (usable and working thought but still) and shouldn't be seen as more until a power user of the actual locking framework reviews it and tells what are real his needs.

The shared lock is what we're missing today, we need it in order to provide an applicative lock (for example, a user locks a node, but he can edit it whatever is the browser and or the page, but others can't); Or develop such features: I already did that for a project, and it is powerfull when you have the need and you use it well.

The API for a shared applicative lock wouldn't be exactly the same thought, because you need to be able to index you lock with additional data, such as UID, an arbitrary token, a group value, etc... It would be a good idea to implement such thing as a layer upper using the atomic lock API thought, but this atomic lock API must be prepared to it.

It would definitely worth another issue for introspecting this idea, but the use case would be more like protecting a node for edition for a particular user, or such, not really for rebuilding cached data.

I also did provide a NULL implementation in the patch, check it out!

Yes, I'm not still convinced that simple mutex and applicative locks should be merged in one single API, but why not, I will try to think about this.

If we add our own fancier lock capability, then I agree, separate issue. If we instead decide to adopt a different locking framework (which based on your post doesn't seem especially likely), the we should of course start with whatever that framework gives us.

I don't fully grok synchronize vs. async future. It's actually been a long time since I did anything with Java threads, so take anything I say with several grains of salt. That said, if we're refactoring the locking API than the ->onAcquire(callable)->onFailure(callable)->execute() model looks really good to me. It's familiar to anyone who's done Ajax programming in Javascript or jQuery, as well as flexible and self-documenting.

Looking through google there is not much at all, in fact this issue is near the top of the google results for several different searches,

Drupal's awesome SEO FTL! :-)

Ok, I think we need more users to tell what the need exactly in order to fine tune the proposal. Also need external reviewers to have non biased opinions. Once we had that, if it fits, is that a go for this refactor in D8 or not?

I'd like to take this patch lead, as I proposed it, and to work with a small set of people as a team for this to work and ensure we'd have a valuable result for the most.

It's a minor API (in term of size and features) but it's critical to have it efficient (as in easy to use, performance of this is mostly impacted by the backend themselves), and most importantly it must be well tested.

I'm with @Damien Tournoud in #46 here,

lock()->acquire('identifier') is very inconsistent with other factories we have.

We use lock('identifier')->acquire() everywhere else.

lock('identifier') is bad DX, because if the WSCCI comes in with DI containers, then this code would not be possible: $container->get('lock')->acquire('identifier') and would then forced to be $container->get('lock')->get('identifier')->acquire(): this is the wrong way to go. In my opinion, the cache() function usage should be changed to cache()->getBin('bin')->get('identifier') so it would be consistent with such container usage.

Remember that procedural function is only a proxy to the manager (in the way I designed it) and if I put the identifier in the lock() function signature, then I have to move some of the business inside the procedural code: then it's not encapsuled anymore in the OOP code and we have to deal with an API splitted on both procedural and OOP: this is a serious offense to every encapsulation principle and a maintainance nightmare.

EDIT: Thanks for the tag.

There is another noticeable difference between cache() and lock(): cache relies on different backends, and you get a different instance (the bin); In that particular use case, you have two identifiers, not just one. When you use the lock backend (here the manager as I called it) there is only one identifier: the lock name.

So, in order to compare both, here is the code for cache:

cache('backend')->get('identifier');

And here it the code for lock:

lock()->getRunner('identifier');

Keeping the identifier itself outside the procedural accessor is what we're doing now, so I don't see any consistency issue with keeping it outside for lock. In fact, making the identifier part of the procedural accessor IS the consistency issue.

Witness:

db_select('identifier')->execute();

Again, a different use case with different meanings of the same words.

The real reason why lock('identifier') is not an option because it will move business code that lies into the OOP code towards the procedural code: it's indirection and obfuscation, end of story. If you have a *real* technical reason to put this identifier there, I would be more than eager to ear it.

I'd be more than pleased to ear constructive criticisms about the whole code, design and patterns instead of being forced to argue a minor and non important detail.

EDIT: I apologize if this was a bit aggressive: this proposal right now still is a *proposal* only, details can be figured out later. The real important point is: does the design looks good? Does it solve most use cases? Finally, how could it be improved and what are the use case it doesn't solve?

Rerolling #55 for the new namespace standard.

I do not at all get the point of the Factory class. It seems like it's spawning backend instances? Or something? I don't see how that fits into a direct PSR-0-ification of the lock API, even with the addition of backends.

I agree with catch that we should probably just get this in with as few changes as possible, then in a separate patch refactor/gut the API. That at least gets us more OO code to clean up later. If not, the we should say to hell with the existing API entirely and start from scratch. For that reason I'm confused by the factory's doWhenAcquired() method, which seems to only make sense in a new-design version that is not complete here.

As far as the future API goes:

1) I can think of no use case to have more than one lock engine active on the same site at the same time. I can, however, see how it would complicate things unpleasantly to do so. Let's not do that.
2) Let's assume that we get to a point where there is a lock manager object, which given point 1 *is* the "backend" (although I hate that name). On that, you would simply want to say "do this, but make it synchronous".
3) So from an API perspective, where we want to end up is:

$lock_system->lock($key, function() {
  // Do if successful
}, function() {
  // Do if not successful
});

In that case, we have a single method, lock(), which takes two callables. And it just happens.

If we wanted more self-documenting methods, then we would have to introduce an intermediary LockCommand object. To wit:

$lock_command = $lock_system->lock($key);
$lock_command->onAcquire(function() {});
$lock_command->onFail(function() {});
$lock_command->execute();

Which is of course also easily chainable. In that case, $lock_system corresponds to a DB connection object, and $lock_command to a query object. I personally like the self-documenting capabilities of the latter, even though it has an extra object.

4) As far as a function wrapper goes, pounard is correct that we should treat it as a temporary hack until we get a better dependency handling system in place. lock() should do nothing except manage a pseudo-singleton that is $lock_system, and expect to get thrown out in favor of whatever we end up with for DI instead. The only parameters a lock() function would take would be for mapping to different backends at runtime... which per point 1 above we do not want.

I did the manager to get out procedural code of procedural layer, by doing this it only removes some functions I could have put in the lock driver interface then implement as a base class, instead, this "common driver code aka the factory" (probably a bad name) is fixed into an additional layer that won't consume more memory or CPU (maybe except the cost of 2 extra function calls per complex use case) but which removes this responsabilities of the driver. It just make the driver lighter and easier to implement, because "as I driver developer, I don't care about the runner and stuff above" (nice user story there), and "as a user I dont care about the driver I just want to use a generic and fully featured API" (another one, the driver is fully hidden for the user here).

Another reason why I kept this "factory/manager" pattern: it's something I was hurting to when I did an evil custom micro framework (for education purpose only): as soon as we have a DIC or SL, this component cannot have polymorphic accessors so we need an extra layer to expose multiple functions (pretty much as the cache has, except that in the cache case, this extra layer remains in the procedural API now, but if we move it to a DIC/SL we will need to add this extra layer, or refactor the full API).

All this common code is easier to test and will be more robust as long as we keep it out of the driver and as long as the driver actually respects the interface contract: it gives us separation between the lock API and the lock driver and allows us easier testing, easier code comprehension, and better and more specific documentation for both of them.

I agreed with the do it all method, it sounded appealing and I started this way in the beginning, but I did go backward for the same reason I'm proposing to explode t() into 4 functions: it makes the code being more explicit at first sight. Having a huge polimorphic function makes its harder to understand, and harder to test, harder to use well, and dynamism also makes the lot potentially slower (we have to check arguments etc..). So I had two possible choices: either expose multiple lock functions, either expose the runner directly, I choosed the second option because handling the complexity into a single responsability runner was easier and avoided a lot of copy/pasted code.

Just a note, the opposite direction is possible, make a "cover all" driver and expose it directly, this would add the following potential benefit: the driver could implement its own runner or implement the API fully for performance reasons. But in the end, it would also give us these limitations: the driver would be harder to port if we modify the interface (with only 4 atomic functions we won't change them every now and then while the API can), and harder to understand what it must do, what it can do, and what it shouldn't do: so driver developers would have far more work than just implementing the four atomic functions. We would potentially lose a lot of backend developers, my guess is some of them are not Drupal zealot but people maintaining a site without knowing internal Drupalism and that create drivers for their environment in a pragmatic fasion: if it's easy, do it, if not, change the CMS.

Don't think I'm taking this "hard": this is only the reasoning that got me to this code, this is not final and I think I agree with all when I say that catch is right, and we should make the first patch minimal. That's why I'm taking everyone's arguments (Damien, sun, catch, Crell) and trying to make the most objective synthesis in order to open a new bug.

Ok so for fun I reworked the patch that passes on my box at least the locking framwork tests, that's for fun. I'm working on whipping out every new stuff and just extract the lock backend as an interface in order to make everyone happy, for next patch!

There should be one warning when the patch applies, but it applies.

EDIT: Arggg.... it attached one incomplete one with...

Mouarf malfunctioning bot... Anyway...

#81: lock-oop-interface-with-runner-PSR0.patch queued for re-testing.

Opened a more atomic change issue #1477446: Move lock backend to PSR-0 code for porting the lock backend to PSR-0 without adding any new API.

Maybe use the Event Dispatcher that's coming with the Kernel patch? #1463656: Add a Drupal kernel; leverage HttpFoundation and HttpKernel

Yes, that's the primary goal, I started this issue long before the container was figured out, but it was planned to include one.

Here is a new and lighter patch porting the LockToken and LockRunner classes in the new lock as PSR-0 patch commited earlier. They are the same as in my upper patches, with some typo and consistency errors fixed.

A new unit test file for this high level API is provided with it, with fake backend implementations for testing, so it does not need to be merged with actual database backend tests (the already existing ones).

It does'nt change any core API and the LockManager (or factory) class has not been ported into this patch (which means it lacks accessors to get the runner and token instances), since it was one of the most criticized points, I'll let this factory piece for later, in order to discuss it into the dependency injection container.

Still, this propose the two first pieces of a higher level API and those pieces are the one to discuss, so please, any reviews, ideas, use cases I didn't see?

Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
WTF? Any ideas?

Sorry erroneous status change.

#91: 1225404-91-Runner-Token-moretests.patch queued for re-testing.

Better^^ Please anyone, now shoot about code design, what it solves, what it doesn't solve, and what's missing or why it should be done totally differently!

I just stumpled across this issue (while rerolling the patch for #1376778: Consistent 'duplicate key' detection in core). In particular the LockRunner stuff seems nice and fulfills some of the ideas laid out in #1293968: Support graceful caching. It think this is very exciting and highly useful, especially on high-traffic sites.

Here is some comments after a first look on the latest patch:

LockToken

I think the LockToken instance should contain not only the name but also the lockId, and both values should passed to method calls on the backend rather than having the backend getting the lockId from a static variable. This will also automatically make the API suitable for releasing locks held by other requests (as suggested in #1266568: Allow releasing a lock acquired by another process).

Note that lock_acquire() currently not only acquires a lock but also renews existing locks. So we also need a LockToken::renew() method. I guess it would be useful to create a separate renew() method on the backend too - in DatabaseLockBackend::acquire() the code paths for acquiring and renewing locks are completely separate.

I'm not sure what the LockToken::wait() method does. If we have a LockToken instance, we have the lock, so there is nothing to wait for.

Rather than having two functions, disableAutoRelease() and enableAutoRelease(), I suggest having just one with an argument, setAutoRelease($enable).

LockRunner

Instead of setWaitDelay() and setTimeout(), how about using the function names setWaitTimeout() and setLockTimeout()? Here "timeout" consistently refers to a maximum timespan ("timeout" is more standard API lingo than "delay"), and setTimeout() is ambiguos because there are actually two different timeouts used.

LockRunner::WAIT_INFINITE seems to belong in LockBackendAbstract. Other code than the lock running may want to wait for a lock indefinitely.

The wait and acquire logic in the beginning of LockRunner::execute() is also generally useful and could be moved to LockBackendAbstract.
When self::WAIT_NONE == $this->waitDelay, wait() is called without a delay, i.e. it defaults to 30 seconds — why not specify a very large number here?
The logic for when self::WAIT_NONE != $this->waitDelay seems a bit too simple: it only waits and tries to acquire the lock once, i.e. it may give up before the maximum wait delay hasn't been reached. I think it should try several times (calling wait() with a delay that reflects $this->waitDelay minus the time already spent, similar to the timeout handling in drupal_http_request()).

In execute() when the success and failure functions are called, the code can be compacted a bit. call_user_func() is just a special case of call_user_func_array() with the second argument being an empty array.

The need for stampede protection often occurs in connection with filling a cache. The workflow is cache->get(), if empty, then rebuild and cache->set(). One example is variable_initialize() — the logic here is rather delicate and AFAIR it took quite some time to get it right. It would be very useful to have this cache-get/cache-set/locking encapsulated in LockRunner. This was one of the goals of #1293968: Support graceful caching. There should be a method for specifying the cache bin and cid, e.g. LockRunner::cache($bin, $cid), and LockRunner::execute() should start by calling cache($this->bin)->get($this->cid) up that cache entry and just return it if it exists. On success it should call cache($this->bin)->set($this->cid, $ret).

Another goal of #1293968 was graceful caching, i.e. instead of clearing a cache entry (or the entire cache bin) which causes all following requests to wait for it to be rebuilt (like in variable_initialize()) it would often be better to mark the cache as invalid but still allow future requests to proceed using the stale value instead of waiting for the fresh value to be recalculated. In theory variable_initialize() might benefit from this, though in practice it is most useful for long-running functions (it may be hard to understand the consequences of allowing a certain function to return stale data, so for short-running functions it may be easier just to have all requests wait 100 ms second for the result). To get the most benefit out of this, it should use a cache bin that is not cleared very often (pressing the “Clear all caches” button should probably just mark all entries as invalid). To support such invalidation we should either extend the cache API or ­­— perhaps simpler and better — simply store the expiry time (i.e. the time when the cache entry is considered stale) together with the cached entry (or possibly in a separate cache entry in a separate bin so that one cache bin can be invalidated by flushing another cache bin). API-wise we need methods for specifying the expire time (cache entry is stale after this period) and possibly also the maximum allowed time (cache entry should never be returned after this time), and also a way of invalidating caches, e.g. LockRunner::invalidate($cid) and LockRunner::invalidateMultiple($cids) (similar to delete() and deleteMultiple() in cache backends).

Well, bedtime for me.

Some random answers:

Note that lock_acquire() currently not only acquires a lock but also renews existing locks. So we also need a LockToken::renew() method. I guess it would be useful to create a separate renew() method on the backend too - in DatabaseLockBackend::acquire() the code paths for acquiring and renewing locks are completely separate.

Not sure this is something we want to expose, anyway this behavior is a direct inheritance from the legacy core lock API. The real question is do we need the developer to be able to renew locks explicitely? In which use cases this would be useful?

Instead of setWaitDelay() and setTimeout(), how about using the function names setWaitTimeout() and setLockTimeout()? Here "timeout" consistently refers to a maximum timespan ("timeout" is more standard API lingo than "delay"), and setTimeout() is ambiguos because there are actually two different timeouts used.

You are right here.

Rather than having two functions, disableAutoRelease() and enableAutoRelease(), I suggest having just one with an argument, setAutoRelease($enable).

Both are equivalent, it's just a matter of religion. I recently read an article about the boolean parameter obfuscation and risks somewhere (cannot find it anymore) which was about C++ code, taking as examples some non explicit QT method names. It may have influenced me. In this particular case, those methods are meant to be called only once and chained, having two methods here actually makes the code somehow a bit more readable. I wouldn't mind merging them, let's wait for other people's feeling about this.

LockRunner::WAIT_INFINITE seems to belong in LockBackendAbstract. Other code than the lock running may want to wait for a lock indefinitely.

This constant may even belong to the package itself and not to a class. I would rather create a Lock class in the namespace root for carrying those constants (to emulate an enum).

In execute() when the success and failure functions are called, the code can be compacted a bit. call_user_func() is just a special case of call_user_func_array() with the second argument being an empty array.

Actually from various benchmarks, call_user_func() is faster than call_user_func_array() and semantically the later is the real special case of the first, when we don't know the argument numbers. This API is meant to be used with lambda functions more than with user set arguments (which is the special case) so I prefered to optimize for the common use case it was designed for. Simplifying to call_user_func_array() in every case may produce fewer lines of code, but I would want to be sure by benchmarking the difference would not be significant first.

There should be a method for specifying the cache bin and cid, e.g. LockRunner::cache($bin, $cid), and LockRunner::execute() should start by calling cache($this->bin)->get($this->cid) up that cache entry and just return it if it exists. On success it should call cache($this->bin)->set($this->cid, $ret).

I'm not sure following you here. If you are proposing to introducing coupling between cache and lock, this goes against the initial design which is to have a decoupled and standalone lock handling API. The cache might be dependent on it but I'm not sure the opposite makes sense.

Still have some of your review notes non answered, I will do it later thought. Thanks for the review.

The real question is do we need the developer to be able to renew locks explicitely? In which use cases this would be useful?

Assume you need to process, say, 100 items in a foreach loop. The processed of each item may take up to 10 seconds but usually takes only 1 second. Either A) you can require a lock for 100 × 10 = 1,000 seconds, or B) you can require it for just 10 seconds and renew it 100 times (once in each iteration). In case your job dies without removing its semaphores (this sometimes happens — that is why we need to specify a lock timeout instead of holding the semaphore indefinitely until it is explicitly released), the semaphore may be unavailable for up to 1,000 seconds in case A, but only up to 10 seconds in case B.

I'm not sure following you here.

Heh, I guess I started out with a pretty basic suggestion and then got carried away with all kinds of whistle and bells extremely loosely explained :-)

My first suggestion is to make a utility function in LockRunner for getting and setting cache entries, because this is a very common use-case. If this behaviour is requested (by calling ->cache($bin, $cid)), LockRunner::execute() works like usual but with these additions:

1. At the very start, check the specified cache entry using cache($bin)->get($cid); if it exists, return it and do nothing more.
2. When the lock has been successfully acquired, check the cache entry once more to see if it has been added by another request; if it exists, return it, release the lock and do nothing more.
3. When the onSuccess has run but before the lock is released, store the return value in the cache using cache($bin)->set($cid, $ret).

This allows variable_initialize() to be written like this:

function variable_initialize($conf = array()) {
  lock()
     ->getRunner('variable_initialize')
     ->cache('boostrap', 'variables')
     ->onSuccess(function() {
        return array_map('unserialize', db_query('SELECT name, value FROM {variable}')->fetchAllKeyed())
     )
     ->execute();

  foreach ($conf as $name => $value) {
    $variables[$name] = $value;
  }

  return $variables;
}

Addition to above suggestion:
I believe the logic described above with a modest effort be extended to eliminate the race condition that currently exists in variable_initialize(). This is one thing that is very easy do to wrong, so encapsulating it in a utility function would be very useful. The race condition is described in #249185: Concurrency problem with variable caching leading to cache inconsistency (the ticket is marked as a duplicate of #973436 that solves the problem using Drupal\Core\Utility\CacheArray, but AFAICT the race condition is just moved to CacheArray::set()).

Then I also briefly mentioned some further additions (the graceful caching), but let's discuss the basic concepts first.

If you are proposing to introducing coupling between cache and lock, this goes against the initial design [...]

In my eyes, LockRunner is a utility class built on top of the locking system (implemented by some Lock backend). With this suggestion it could also optionally use the caching system (implemented by some Cache backend). The locking and caching systems should be kept separate. So I don't think my suggestion represents a coupling. Possibly the caching logic could be kept separate in a subclass or implemented using some callback mechanism.

Regarding microtime(), see also:

• #1416878: PDOException: Numeric value out of range (negative timer value / potential microtime issues)
• #838976: lock_acquire() breaks if new expiration is the same as old one

It would really like to see some of this in D8. Would it be beneficial to split this into two tickets? One ticket for the adjustments to the existing lock API (the introduction of LockToken etc.) and one for the LockRunner?

About LockRunner: I think this is a really strong concept, especially with the async option mentioned above (i.e. the ability to spawn tasks to be run in the “background” with stampede protection and what not). Note that sometimes you may want to run such jobs without stampede protection but just delayed/queued, e.g. similar to what is offered by the worker callback in hook_cron_queue_info() — so I think we should choose a different name than LockRunner. Perhaps something like this: task('menu_rebuild')->lock()->async()->start().

Attached a patch rerolled for current 8.x codebase.

• Moved the unit test class into the new tests PSR-0 structure
• Renamed setWaitDelay() to setWaitTimeout() and setTimeout() to setLockTimeout() as suggested in #97.

Need eyes for review, and please express any non covered needs.

@#101 I do not agree with renaming the LockRunner as Task, PHP cannot do asynchronous processing, there is no sense in emulating that in the names: this is why I think this should remain lock specific code.

EDIT: Note that the current patch does not modify the original locking framework, it just adds a new layer which needs to be manually instanciated. Once we'll have fixed the implementation and API details, we can proceed into removing the lock.inc file and move the remaining lock() accessor into the the bootstrap.inc file or in the DIC.

I do not agree with renaming the LockRunner as Task, PHP cannot do asynchronous processing [...]

By asynchronous I mean something like Background Progress (run jobs in a parallel HTTP requests) or hook_cron_queue_info() (run jobs in sequence on one parallel HTTP request).

Example use case: On Drupal.org, when somebody posts a comment in a ticket, mail is sent to all people following that ticket. If a ticket has a huge number of followers, this may take a long time. To avoid that the person submitting the comment form should wait for the mail sending to complete, you want the mail sending to happen in some later HTTP request.

All comments should trigger mails to be sent, so there is no need for stampede protection. Of course you could circumvent this by assigning a unique lock key for each comment (in practice removing the concurrency check), but that seems like a workaround.

You could use hook_cron_queue_info() for this use-case. However, the API for processsing tasks in a queue is very different from the proposed LockRunner API.

I see, but lock processing is never meant to run background operations, these are two very different use case IMHO. The task API is still a good idea, but a whole new idea which needs some thoughts.

Here is another patch for sample purposes:

• Added back the LockManager class, wired it with the LockToken class.
• Refactored some sample core lock usage to use it.

But it still might be a good idea to keep this use case in mind (i.e. background processing), so that we can consider cross-request locking already at this time (we don't have that yet in D8, do we?).

All locks are released when the page exits using the shutdown handlers: any long run operation in CLI will keep its lock while running but there is no applicative level locking API. Don't know if this answers your question.

It does. Thanks. So we should be careful not to shut us off from making cross-request locks in the future.

Writing a stronger application level locking API is something that I want since some years, I already did it more than once in contrib modules, but we could do it in core quite easily.

Fixed legacy functional tests.

Anyone attending DrupalCon in Munich? Then let's talk.

http://munich2012.drupal.org/content/event-based-lock-framework-asynchro...

I won't be in Munich.

Ok :-(. Hope some are able to be there, and we'll return with all the input and ideas to this thread.

I admit to skipping a bunch of comments that I was way behind on reading. I dove right into the patch and just gave a straight review of using the code.

+++ b/core/modules/system/lib/Drupal/system/Tests/Lock/LockUnitTest.phpundefined
@@ -0,0 +1,211 @@
+    lock()->doWhenAcquired('lock_a', function () use ($backend, &$was_free) {

Where is this method defined? I don't know how those tests aren't throwing fatal errors because I can't find it in the patch. They only seem to be used in the patch so I'm assuming cruft from earlier patches.

+++ b/core/includes/bootstrap.incundefined
@@ -940,19 +940,21 @@ function variable_initialize($conf = array()) {
+      ->onFail(__FUNCTION__, array($conf))
+      ->execute();

Something about the way this onFail logic changed makes me nervous. Is it building a possibly unbounded recursive stack of function calls? I like the idea of the chained logic otherwise with a caveat I'll get back to.

+++ b/core/includes/lock.incundefined
@@ -83,9 +128,11 @@ function lock() {
+
+    $lock_manager = new LockManager($lock_backend);

This surprised me when I got down and found out these are not plugins. Why? They seem to have the same setup, even some of the same terminology.

So back to chaining confusion. It seems confusing that sometimes we're calling functions on the manager object through lock() and other times we're requesting factoried objects. Seems like this could be more clear or abstracted better or something. I'm not sure.

There's this.

lock()->acquire('menu_rebuild');

Then there is getBackend() calls through to a backend I guess. acquire() is just a special case since we sometimes call acquire on the backend through this method? Seems a bit WTFy.

lock()->getBackend()->...

And then getRunner is a factory for a different object. Not really clear from the chain indentation where its used that we're working on a different object. I think I see the distinction but I wonder if the factory is necessary or just confusing things. Could we just instantiate the runner with a backend? The only thing really abstracted away is the timeout and I think that's actually a bad move.

lock()->getRunner('variable_init')->...

Where is this method defined? I don't know how those tests aren't throwing fatal errors because I can't find it in the patch. They only seem to be used in the patch so I'm assuming cruft from earlier patches.

Good catch, I have to see into this.

Something about the way this onFail logic changed makes me nervous. Is it building a possibly unbounded recursive stack of function calls? I like the idea of the chained logic otherwise with a caveat I'll get back to.

This actually reproduces the exact same algorithm as the actual stable core function, no less, no more. It's bit tricky because the original method is, it actually does call itself recursively. I'm against keeping it that way, but it was only in order to see that it was possible to achieve using the proposed API.

This surprised me when I got down and found out these are not plugins. Why? They seem to have the same setup, even some of the same terminology.

Inheritance from the earlier patches which were here a long time before the plugin API was. I hesitated to rename it factory instead. But somehow plugins might be good to use, but I'm afraid that it would cause a circular dependency somewhere because locking API is used very very early in bootstrap sometime in some cache backends. Maybe you could enlighten me here?

So back to chaining confusion. It seems confusing that sometimes we're calling functions on the manager object through lock() and other times we're requesting factoried objects. Seems like this could be more clear or abstracted better or something. I'm not sure.
[...]
Then there is getBackend() calls through to a backend I guess. acquire() is just a special case since we sometimes call acquire on the backend through this method? Seems a bit WTFy.

The getBackend() method is public, but is not supposed to be used by API end users. The acquire() method is when you want to deal with release operation manually (which is totally legal) and gives you a token that allows you to deal with the various lock operations; The runner is a big helper that provides via chaining readable code, and more than that, avoid the need for you to proceed to error handling by yourself thanks the onFail helper. Those are the only three operations you can do with the factory, which is not so many, and each one of them gives you a different component you can then use as you wish.
As soon as you read the return type hinting or use a good IDE, everything seems clearer I guess.

And then getRunner is a factory for a different object. Not really clear from the chain indentation where its used that we're working on a different object. I think I see the distinction but I wonder if the factory is necessary or just confusing things. Could we just instantiate the runner with a backend? The only thing really abstracted away is the timeout and I think that's actually a bad move.

Maybe you are right. But the runner is not a factory, it is a simple object instance, usable only once. The factory is here because in a late future, I guess, it would be into the DIC and some object, at some point, needs to carry the backend reference and be here to create the runner or lock tokens.
The idea of not instanciating the runner with the backend is in order to have the simplest backend implementaiton possible, with a minimalistic set of primitives, which makes it really easy to implement on most systems. Maybe this is not a good move and maybe the runner could be backend specific too, I'm not sure if this would be a good move too. Any ideas?

Needs issue summary

It would really like to see some of this in D8. Would it be beneficial to split this into two tickets? One ticket for the adjustments to the existing lock API (the introduction of LockToken etc.) and one for the LockRunner?

I went ahead and created a separate ticket about representing acquired locks as instances of a new Lock class: #1906772: Make locks objects that auto-release during garbage collection

Updated issue summary.

Is this issue still valid?

I'm not really sure where D8 is on the lock issue, maybe this worth the shot of investigating it for a future minor release.

I'm pretty sure this wouldn't be approved for 8.0.x. Some implementation approaches might be 8.1-safe, or there might even be an existing library we could potentially use.

I meant 8.1 or later too, sorry.

I like the proposal in #112 :).