Move configuration import lock to lock.persistent service since a lock can not exist beyond a single request

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

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.

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).

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.

I'm gonna try this.

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

Um.

Part 2!

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

/me is stumped.

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).

I will re-roll this.

I re-rolled the patch from comment 12.

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).

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...

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.

@Xen

Not quite sure how I managed to break my patch

Added missing use statement.

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.

@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!!!

26: move_configuration-2205527-26.patch queued for re-testing.

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.

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.

@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.

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.

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.

Reroll.

We will add tests for PersistentDatabaseLockBackend in this issue.

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.

Added the semaphore table to FieldImportDeleteUninstallTest.

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

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.

Fixed.

+++ 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 :)

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 ...

Reroll.

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.

Back to RTBC!

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

    // 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.

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 ...

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.

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.

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

nit - typo: use -> used

Besides that:

- Back to RTBC :).

Thanks.

Committed/pushed to 8.0.x, thanks!

A critical bug in this functionality was found: #3204073: ConfigImporter acquires a lock for 30 seconds but this might not be long enough.