Problem/Motivation
Since #3441549, ContentLock::locking() acquires a lock from the lock service to make the fetch-then-merge sequence atomic. When that semaphore cannot be acquired, execution falls through to the last else branch, which tells the user:
This content is being edited by another user.
and returns FALSE, which makes Hook\FormAlter::formAlter() disable the whole entity form.
That branch is reached when the semaphore is unavailable and no valid lock is held by another user, since that case is handled by the branch above. In other words it is reached when there is no lock at all, when the lock belongs to the current user, or when a foreign lock is stale. In all three cases the message is factually wrong: nobody is editing the content.
This is not only a wording problem. DatabaseLockBackend::acquire() is called without an explicit timeout, so a semaphore row left behind by a request that died — fatal error, client abort, worker killed — stays valid for up to 30 seconds. During that window every user opening that entity gets a disabled form and a message blaming an editor who does not exist. Sites that refresh the lock from JavaScript get the same false negative on every refresh.
Note that the preceding branch already decides "locked by another user" without needing the semaphore. The fallback therefore conflates two different things: "I cannot safely write to the content_lock table right now" and "somebody else is editing this content".
This looks unintended rather than by design: the discussion in #3441549 is entirely about the race condition between the select and the merge, this fallback is never mentioned there, and no test covers it.
Steps to reproduce
- Enable content lock for nodes.
- Make sure no lock exists for the node you are about to edit:
TRUNCATE content_lock; - Simulate a semaphore left behind by an interrupted request, as the lock service would have created it:
INSERT INTO semaphore (name, value, expire) VALUES ('content_lock:node:1', 'left-over', UNIX_TIMESTAMP() + 60); - Open
/node/1/editas any user with edit access.
The form is disabled and shows "This content is being edited by another user", while the content_lock table is empty and nobody is editing the node.
Proposed resolution
Only ever report the content as locked when a valid lock is actually held by another user. When the semaphore is unavailable and no such lock exists, do not block the user. Several options, in increasing order of intrusiveness:
- Release a stale foreign lock if there is one, save the lock best effort, and return
TRUE. The window in which two users could both proceed is bounded by the semaphore lifetime, and is far narrower than the up-to-30-seconds window during which the current code blocks everybody. Sites using theconflictmodule still detect concurrent saves. - Return
TRUEwithout writing the lock at all, letting the next request refresh its timestamp. Safer with regard to the original race, but leaves the entity unlocked for a moment. - Keep returning
FALSE, but stop claiming that another user is editing, and give callers a way to tell "locked" from "temporarily unavailable".
The merge request implements option 1, which is what we run in production. I am happy to reshape it into whichever option maintainers prefer.
Remaining tasks
- Agree on which of the options above is wanted.
- Merge request against 3.x.
- Test coverage for that fallback: with the semaphore held by another lock id and no valid foreign lock,
locking()must not report the content as locked. No existing test exercises this path. - Review whether the same reasoning applies to
Hook\FormAlter::contentModerationEntityFormAlter(), which forbids access to the moderation form on the same basis.
User interface changes
Users no longer see "This content is being edited by another user" when no user is editing the content, and the entity form is no longer disabled in that case. No visible change when a valid lock is genuinely held by somebody else.
API changes
None. ContentLockInterface::locking() keeps its signature; only the value it returns in the semaphore-contention case changes.
Data model changes
None.
Issue fork content_lock-3613884
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #3
nono95230 commentedMerge request opened against 3.x, implementing option 1 from the summary, with a
kernel test covering the three cases: no lock, own lock, and a valid foreign lock
which must still block. No existing test exercised this path.
Happy to switch to option 2 or 3 if maintainers prefer a different trade-off.