Problem/Motivation

#2004370: Batch the configuration synchronisation process enabled the configuration import process to use the batch process. Unfortunately this broke the locking mechanism since Lock::releaseAll() will remove any locks at the end of the first batch request.

Proposed resolution

Create a lock.persistent service. The current implementation extends database lock backend and makes it persistent by making the lock id a constant and not registering a shutdown function during construction. The lock must be able to exist across requests unlike the current database implementation.

Remaining tasks

Review patch

User interface changes

None

API changes

None - the lock service in config importer is replaced with the new lock.persistent service but they implement the same interface.

Files: 
CommentFileSizeAuthor
#75 2205527.75.patch11.82 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,399 pass(es). View
#75 interdiff-71-75.txt774 bytescilefen
#71 2205527.71.patch11.82 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,095 pass(es). View
#71 70-71-interdiff.txt1.1 KBalexpott
#70 2205527.70.patch11.64 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,067 pass(es). View
#70 64-70-interdiff.txt2.94 KBalexpott
#64 59-64-interdiff.txt1.08 KBalexpott
#64 2205527.64.patch10.09 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,089 pass(es). View
#59 2205527.59.patch9.76 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,080 pass(es), 0 fail(s), and 2 exception(s). View
#54 2205527-54.patch10.54 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2205527-54.patch. Unable to apply patch. See the log in the details link for more information. View
#53 interdiff-51-53.txt492 bytesmartin107
#53 2205527-53.patch10.54 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,582 pass(es). View
#51 interdiff-44-51.txt496 bytesmartin107
#51 2205527-51.patch10.55 KBmartin107
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,582 pass(es). View
#45 2205527-44.patch10.55 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,574 pass(es). View
#45 interdiff-41-44.txt1.81 KBcilefen
#41 2205527.41.patch9.71 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,564 pass(es), 0 fail(s), and 2 exception(s). View
#41 40-41-interdiff.txt5.94 KBalexpott
#40 move_configuration-2205527-40.patch5.83 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,250 pass(es). View

Comments

alexpott’s picture

Title: Move configuration import lock to state since a lock can not exist beyond a single request » Move configuration import lock to KeyValueExpirable since a lock can not exist beyond a single request
Issue summary: View changes
pounard’s picture

Shouldn't this be "add an applicative lock API then use it" ?

catch’s picture

I was pondering that as well, the implementation is pretty much exactly the same as the existing lock API, just without the end of request cleanup. Seems worth having - will be other cases where you want to lock for the duration of a batch.

pounard’s picture

It should probably have its own API (pretty much more or less the same as lock API) with some additional properties to the lock (owner identifier, group identifier, etc...) that can be used to provide multiple locks release based on owner (logout or session drop) and based on groups/token (use cases I had more than once).

catch’s picture

mikeytown2’s picture

Could we get away with passing in a custom value to DatabaseLockBackend::acquire & DatabaseLockBackend::release instead of using LockBackendAbstract::getLockId?

What ever we do decide to do, having a cron job that cleans up expired locks would be needed.

Above is essentially what I do in HTTPRL and works because I use the lock to verify that the new process should be able to run. It then removes the lock and continues on from there. Generalizing this workflow might take a new API that closely resembles the current locking api; really depends on how it is used.

Gaelan’s picture

Assigned: Unassigned » Gaelan

I'm gonna try this.

Gaelan’s picture

Here's a start. It adds a PersistentLockBackend, but it doesn't do anything with it yet.

Gaelan’s picture

FileSize
6.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Um.

Gaelan’s picture

FileSize
7.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
7.34 KB

Part 2!

Gaelan’s picture

Status: Active » Needs review
Gaelan’s picture

FileSize
7.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,786 pass(es), 2 fail(s), and 0 exception(s). View
3.35 KB

Add changes suggested by @alexpott. NOTE: the last interdiff wasn't an interdiff, so this interdiff goes back to #9.

The last submitted patch, 9: drupal.persistentlockbackend.2205527.8.patch, failed testing.

The last submitted patch, 10: drupal.persistentlockbackend.2205527.10.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: drupal.persistentlockbackend.2205527.12.patch, failed testing.

Gaelan’s picture

Assigned: Gaelan » Unassigned

/me is stumped.

pounard’s picture

This should not be called "persistent lock" because it's more a property than its definition, it should be called "applicative lock" or "user lock" instead.

Instead of using a key value store, using a custom implementation in there may be cleaner. Most methods of the key value store won't be used in here anyway, and a lock has really a few methods. Key value store don't really ensure atomicity in their method calls whereas a lock should, even if it's a business lock.

A lot of business is missing in there, there should ways to group locks altogether (locking multiple business objects and releasing all of them at once) as long as a few other properties (such as user identifier, which would allow to drop all locks of a user at once, on logout for example).

martin107’s picture

Issue tags: +Needs reroll
camprandall’s picture

I will re-roll this.

camprandall’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,744 pass(es), 2 fail(s), and 0 exception(s). View

I re-rolled the patch from comment 12.

Status: Needs review » Needs work

The last submitted patch, 20: drupal.persistentlockbackend.2205227.20.patch, failed testing.

Xen’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

As @pounard points out, KeyValueExpirable is unsuitable as a lock store, due to the lack of guaranteed atomicity.

But instead of reimplementing the wheel, we can just reuse the locking logic of DatabaseLockBackend by subclassing it and disabling the releasing of locks when the request finishes.

I've implemented that in this patch (no interdiff, as there's too little commonality).

It's still up for discussion whether it should be lock.persistent or something else, and whether the inheritance shouldn't be the other way around so DatabaseLockBackend is the persistent one and RequestDatabaseLockBackend extends it with clearing locks on request end (and consequently lock.request).

Status: Needs review » Needs work

The last submitted patch, 22: move_configuration-2205527-22.patch, failed testing.

pounard’s picture

I do think that having an applicative lock forces us to reinvent the wheel, you need a few other properties on those locks than only a name, or it's rather useless. For advanced locking I can see a lot of nice features to have:

  • Delete all locks owned by a user
  • Tell a user which other user did lock a node when editing it
  • Allow the user to remove the other user's lock if he has permission to do so
  • Lock all objects you can see on a page (all nodes, views, etc..) using some kind of token (for exemple, inline page editing that goes further than just the node's field, but everything that's visible on the current page)
  • Release them all at once

For all of those uses cases, you need the applicative lock to be have and be queried using:

  • identifier (object locked)
  • session identifier (upon logout, a user can be logged from two differnet places and close only one session, there are valid use cases where he'd keep the lock on the computer is not in front of yet)
  • owner user identifier (user delete, or various other use cases)
  • page specific token (group of locks grouped together for editing purpose or other, you need to be able to release all of them at once)
  • be volatile by having a ttl (but the ttl is not a property you need to be publicly queried, your storage can handle it internally)

For this to be implemented, we indeed need to reinvent the wheel, but I think this is a good thing to have because it paves the way to great editing features. Think of a site where you attempt to edit a node and the UI tells you that some other folk's actually editing it, since when did he lock it, etc...

Xen’s picture

Simple locks is not "rather useless", Drupal has been using them for years, and it's exactly what we need here. More advanced locks is all good, but it's not what we need here, and there's no reason to make simple locks not-simple, in order to support a, as yet, theoretical need. It's quite simple to add a new service for more advanced locks, which those that need it can then use.

Not quite sure how I managed to break my patch, but I need to take a look at the lockId thing again anyway.

martin107’s picture

Status: Needs work » Needs review
FileSize
6.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,581 pass(es), 0 fail(s), and 11 exception(s). View
498 bytes

@Xen

Not quite sure how I managed to break my patch

Added missing use statement.

Status: Needs review » Needs work

The last submitted patch, 26: move_configuration-2205527-26.patch, failed testing.

pounard’s picture

The downside of doing things simple here would be that we'd potentially end-up with three different locking levels (and different API's):
- technical mutex (actual locking system)
- persistent mutex (this one)
- applicative lock (more complex and business oriented one)

So much for not reinventing the wheel and API consistency.

Actually if you want stuff to remain simple I'd suggest that instead of creating a new abstraction for locking just add a boolean status that is "persistent" to actual locking API. This would make things way simpler and won't need to add a new service. It would actually be much simpler to implement, the boolean status don't need to be in database, but just in the PHP thread memory since its only use would be to skip the lock release at request termination.

Doing that, and single and only problem remains: allow the current session to reacquire the same lock (for exemple within a batch), but all you have to do to allow this is to persist the lock id global into session or into the batch. Note that this might cause concurrency problem between two opened browser tabs using the same session, so all you need to do is to add a owner token column, and persist that into the batch.

For example:

if (!empty($batch['arguments']['my_token']) {
  $token = $batch['arguments']['my_token'];
} else {
  $token = true;
}
if ($token = lock_acquire('lock_name', SOME_TIMEOUT, $token /* persistent */)) {
  $batch['arguments']['my_token'] = $token;
} else {
  // Acquire or RE-acquire lock failed
}

Adding a new service, a new API, and making a distinction between those kind of mutexes sound like a useless code and API indirection level.

Xen’s picture

@pounard
> So much for not reinventing the wheel and API consistency.

'cuse me? We're talking one new service, with exactly the same API as the existing lock service. That's not exactly reinventing the wheel, especially as it's using the same code as the existing, but for two lines. It seems to me to be exactly this kind of thing that "services" were invented for.

And while I think your suggestion for an alternative implementation does have some merit, it *is* an API change, as you're extending the acquire method signature.

I think it will be hard to swallow for the bigwigs, as the third argument to the method *drastically* alters its behavior. An alternative would be acquirePersistent, but this changes the API even more, besides being ugly.

By all means, come up with a suggesting for how to implement an advanced locking system, I'm curious how you'll approach the problem. Seems to me that in order to add all kind of things to a lock, we'll have to have a lock object to work on. And a concept of "things that can be locked" instead of the "random string" we have now.

In the meantime, I'll try to get this patch passing. I have half a mind to rename the existing lock service lock.request and fix up ALL THE THINGS!!!

Xen’s picture

pounard’s picture

Sorry I of course didn't meant to be rude, and I do respect your point of view here.

Nevertheless, I'm just suggesting that evolving the new API instead of having yet another service, it sounds odd to have to different locking services which serves almost the same purpose and use the same API.

Xen’s picture

Status: Needs work » Needs review
FileSize
5.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,628 pass(es), 0 fail(s), and 2 exception(s). View
1.35 KB

OK, was a bit overeager in applying lock.persistent. UnitTests shouldn't use it, as the default lock service gets replaced by the NullLockBackend, as there's no database.

The last failure I think is unrelated, as I'm pretty sure I've seen another issue for it.

I still think that it's basically wrong that the default lock is bound to the request, without it being explicit in the class/service name. I'd think that it would be a DrupalWTF for developers coming from elsewhere, already familiar with the lock concept. But that's the kind of change that needs a nod from Damien.

The last submitted patch, 26: move_configuration-2205527-26.patch, failed testing.

Xen’s picture

@pounard
Fair enought, I don't want to be rude either, I'm just coming from the other direction just trying fix the basic issue for the moment being.

Frankly, I don't have an idea as to how a new API should look, but I'm quite interested if someone comes up with an elegant solution. In this case, I'm more of a "fix, then improve" type of guy. As it is, the wishlist is a tad to abstract for me, I need something a bit more concrete before getting creative.

pounard’s picture

No worries, I had written such applicative locks so many times because Drupal core doesn't ship any that I try to make this happen. I agree this might not be the right issue thought. Nevertheless, I don't try to do D8 developement (anymore) because I quite don't like it much (that's personal nothing I really complain about here) but I still follow it as much as I can, and I see there are a lot, really a lot, of services in D8 core today, and by adding yet another one I'm afraid at some point, there'll be so many that module developers won't even know that a persistent lock exists.

Status: Needs review » Needs work

The last submitted patch, 32: move_configuration-2205527-31.patch, failed testing.

Xen’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,840 pass(es). View
537 bytes

Managed to find that last failure with a bit of help from https://www.drupal.org/node/2239969

KernelTestBase needs to take the new lock service into account. Which is a bit ugly, but unless the locking service gets redesigned, it'll have to do.

cilefen’s picture

FileSize
5.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,119 pass(es). View

Reroll.

cilefen’s picture

We will add tests for PersistentDatabaseLockBackend in this issue.

cilefen’s picture

FileSize
5.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,250 pass(es). View

This is a reroll based on a conflict caused by #2148199: Use snapshot to warn users if the configuration has changed since last import in ConfigSync.

alexpott’s picture

FileSize
5.94 KB
9.71 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,564 pass(es), 0 fail(s), and 2 exception(s). View

Added tests for the new persistent lock service - this looks good go.

There is no need to set up the service in the minimal container for KernelTestBase - if this is required for a KernelTestBase test then that test can simply override containerBuild.

Status: Needs review » Needs work

The last submitted patch, 41: 2205527.41.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
9.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Added the semaphore table to FieldImportDeleteUninstallTest.

Status: Needs review » Needs work

The last submitted patch, 43: move_configuration-2205527-43.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
10.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,574 pass(es). View
cilefen’s picture

pwolanin’s picture

Should the persistent lock class have a different interface - at least by name? or an added method so you can confirm it's persistent?

alexpott’s picture

#47 I don't think so. The lack of persistence of the usual lock service can not be detected :) - the clue is in the name. It certainly does not need a new interface.

alexpott’s picture

@cilefen - great find on ConfigImporter::reinjectMe()

swentel’s picture

Looks good to me, only one nitpick in the docs.

+++ b/core/lib/Drupal/Core/Lock/PersistentDatabaseLockBackend.php
@@ -0,0 +1,34 @@
+ * Definition of Drupal\Core\Lock\PersistentDatabaseLockBackend.

We use 'Contains' now.

martin107’s picture

FileSize
10.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,582 pass(es). View
496 bytes

Fixed.

swentel’s picture

+++ b/core/lib/Drupal/Core/Lock/PersistentDatabaseLockBackend.php
@@ -2,7 +2,7 @@
+ * Contains of Drupal\Core\Lock\PersistentDatabaseLockBackend.

Actually no, it's 'Contains nameOfFile' - so the 'of' is not needed :)

martin107’s picture

FileSize
10.54 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,582 pass(es). View
492 bytes

Ah yes conclusive proof that coding before breakfast is never a good idea, now matter how small the change.:)

PS also it needs a \ to be a full qualified name ...

cilefen’s picture

FileSize
10.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2205527-54.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Hm, from a code standpoint I would RTBC it, but that the backend is fixed to database feels a little strange.

Maybe we can inject the lock service instead with a special parameter ($persistent = TRUE / FALSE) and implement the interface and proxy the calls through?

I am putting it to RTBC to see what core committers think.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: 2205527-54.patch, failed testing.

chx’s picture

Fabianx’s picture

Title: Move configuration import lock to KeyValueExpirable since a lock can not exist beyond a single request » Move configuration import lock to lock.persistent service since a lock can not exist beyond a single request
alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
9.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,080 pass(es), 0 fail(s), and 2 exception(s). View

re #55 it's a bit tricky. We can't just proxy to the existing lock service since the shutdown function registered in the constructor and the fact we can't change the lock id in the same way. I think it is ok for this to be built by extending DatabaseLockBackend - if someone builds an alternative lock service then they can also build an alternative persistent lock service.

Status: Needs review » Needs work

The last submitted patch, 59: 2205527.59.patch, failed testing.

alexpott’s picture

Issue summary: View changes

Status: Needs work » Needs review

alexpott queued 59: 2205527.59.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 59: 2205527.59.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
10.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,089 pass(es). View
1.08 KB

We should make the ConfigImporter less opinionated on what services it has injected.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC!

Thanks for the feedback, if core committers agree, this is a good approach I am okay, too.

chx’s picture

Status: Reviewed & tested by the community » Needs work
    // Set the lockId to a fixed string to make the lock ID the same across
    // multiple requests.
    $this->lockId = 'persistent';

but that will not merely persist across multiple requests but multiple types of lock requesters as well.

alexpott’s picture

Status: Needs work » Needs review

@chx but we need the lock id to be a constant otherwise persistent locking will not work. Maybe you just want the comment updated? But I'm confused since doesn't the implementation of LockBackendAbstract::getLockId() also make the lock id persist across multiple types of lock requesters too?

@Fabianx to make the persistent a proxy to the database lock service we need to do a lot of work on the database lock service. And even then it could not proxy directly to the service it would have to clone it since we need to have a different lock id for the persistent and non-persistent service.

Fabianx’s picture

I got confused about the persistent, too.

If I understand this correctly, this is just a "prefix" that is appended / prepended to the actual lock ID, so its unique per request or persistent like here.

But I might be wrong, it definitely is confusing ...

catch’s picture

  1. +++ b/core/lib/Drupal/Core/Lock/PersistentDatabaseLockBackend.php
    @@ -0,0 +1,34 @@
    +    $this->lockId = 'persistent';
    

    Agreed on the feedback above - I'm not clear what this is for, given

  2. +++ b/core/modules/config/src/Tests/ConfigImportUITest.php
    @@ -228,14 +228,14 @@ function testImportLock() {
    +    $this->container->get('lock.persistent')->acquire($config_importer::LOCK_ID);
    

    lock ID being a method argument here.

alexpott’s picture

FileSize
2.94 KB
11.64 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,067 pass(es). View

re #69:
1. This is a confusion between lock ID and lock name - lock ID is badly named but this documentation is revealing.

  /**
   * Gets the unique page token for locks.
   *
   * Locks will be wiped out at the end of each page request on a token basis.
   *
   * @return string
   */
  public function getLockId();

2. Here the $config_importer::LOCK_ID is the lock name. Happy to change this constant to lock name to remove some confusion.

alexpott’s picture

FileSize
1.1 KB
11.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,095 pass(es). View

And lets improve the docs to be more explicit and helpful.

cilefen’s picture

Issue summary: View changes
catch’s picture

Both interdiffs look great. I'm also fine with the service database dependency in #62 the use case for swapping this out is considerably less common than the lock system in general, and it's still possible to do if you want.

Fabianx’s picture

> + // multiple requests. The lock ID is use as a page token to relate all the

nit - typo: use -> used

Besides that:

- Back to RTBC :).

cilefen’s picture

FileSize
774 bytes
11.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,399 pass(es). View
chx’s picture

Status: Needs review » Reviewed & tested by the community

Thanks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6ba1da1 on 8.0.x
    Issue #2205527 by cilefen, alexpott, martin107, Xen, Gaelan,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.