Let's make the lock API more OO and more similar to the way database transactions are handled, like this:
The return value from lock()->acquire() should be an object with $lock->renew() and $lock->release() methods. If a lock object goes out of scope, it is automatically released.
This eliminates the need for the shutdown function and the lock()->releaseAll() method called from the shutdown function.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | lock-object-5.patch | 34.3 KB | c960657 |
| #7 | lock-object-4.patch | 33.39 KB | c960657 |
| #5 | lock-object-3.patch | 33.49 KB | c960657 |
| #3 | lock-object-2.patch | 31.73 KB | c960657 |
| lock-object-1.patch | 29.52 KB | c960657 |
Comments
Comment #2
olli commentedMissing Lock.php?
Comment #3
c960657 commentedSorry, this patch includes Lock.php.
Comment #5
c960657 commentedComment #7
c960657 commentedComment #9
c960657 commentedComment #10
c960657 commentedBonus feature:
The code for acquiring and renewing locks is now split into two different methods. Today they are completely separate code paths but wrapped in the same function.
Positive side-effects that makes the Lock system more powerful without sacrificing simplicity or DX:
Comment #11
sun1) I really like the architectural idea of Lock objects.
2) We cannot and must not rely on __destruct(). This is why: #843114: DatabaseConnection::__construct() and DatabaseConnection_mysql::__construct() leaks $this (Too many connections) — Tests are affected, too.
Comment #12
c960657 commentedI don't understand your concern. #843114 was about destructors not being called due to circular references. This is not the case here, at least not unless the lock objects are stored in some static variable or similar so that garbage collection does not occur. I assume that even in the case of circular references, the destructor is eventually called at the end of the request?
On the other hand, I think it would be a good idea to make it preferred convention in core to always call $lock->release() method explicitly. This would make it more transparent what is going on.
Comment #13
Crell commentedThe use of destruct() here is effectively the same as the way we use transaction objects in the database API. Those commit-on-destruct, and since they're local scope they'll die at the end of the function and release then. There's no end-of-request shutdown ordering issues to deal with. I'm not aware of any issues with our transaction code right now, so this should be fine, too.
Comment #14
pounardNote that this issue is a split of #1225404: Modernize lock acquisition to RAII.
I would rename
autoRelease()tosetAutoRelease()to make the method name more consistent.Your patch causes a consistency problem, the backend should keep the
release()low level method without theLockObjectobject in its signature, we should probably leave the backend as it was. The backend can be used without the lock object and should be kept as such.The
Lockclass is an helper that should not be change the backend interface: the simpler the backend stays, the easier it is to implement, plus, many backends in contrib can be ported just by renaming methods today, by making it dependent on theLockclass, we loose easy porting of existing code. TheLockclass doesn't bring anything valuable for the backend itself so it should exist in an upper code layer and leave the backend as it exists today.To solve that a factory injected into the DIC should be used instead, that would handle the
Lockinstances.Comment #15
c960657 commentedAgreed.
Unless there is a common interface defined by PSR or similar, you would have to adjust the third-party code anyway. I don't think that it would not require a substantial larger effort to add support for the Lock class. It does not impose a functional requirement, just a way to wrap input and output arguments.
I can see how it would be possible to decouple the backend from the Lock objects, either by some factory method, or e.g. by reintroducing lock_acquire(), but I don't really see the benefit.
One fundamental difference introduced by the patch is that with the patch you are required to specify the lock ID in order to renew or release a lock. This has the benefit that you don't release somebody else's lock by mistake, and it gives some possibilities with respect to cross-request locks. But of course this may be implemented without passing the Lock class around but simply the lock ID.
Comment #16
c960657 commentedFor the record, I am not convinced that cross-request locks is something we should support. But being able to unlock a stale lock from a separate request is important, I think.
Comment #17
pounardIn actual core (both 7 and 8), as in my previous patch in the other issue, you cannot delete another thread lock thanks to the lock_id condition on all delete queries. The lock id is hidden from the public API just for this reason. I don't see where you patch change or fix anything of that.
Fair enough, we can skip the factory by making the backend manage directly the
Lockinstance, but having a factory arround would make the backend itself easier to implement (only atomic methods with primitive parameters) which decouples it from the upper layer and makes it being really simple and minimal, I think this would be a good idea to keep it this way.Comment #18
c960657 commentedIt does not change the situation with parallel requests. But within a request, you may run into a situation where the code attempts to acquired the same lock multiple times, especially with code that calls itself recursively. But okay, this may be an edge case.
Comment #19
pounardOK I understand your point here. In actual core, for example a recursive function, the lock is renew when acquired a second time. There is the release problem yes, but having such acquire() or release() calls in any kind of recursively called code would be design error. We should not encourage people to write wrong code.
Comment #33
smustgrave commentedThank you for sharing your idea for improving Drupal.
We are working to decide if this proposal meets the Criteria for evaluating proposed changes. There hasn't been any discussion here for 8+ years which suggests that this has either been implemented or there is no community support. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #34
smustgrave commented3 months has past so closing out, but don't worry if still a valid task this can be re-opened by anyone.
Thanks!