Fix: object lock

[Rom1-B]

Checklist before requesting a review

Please delete options that are not relevant.

• I have read the CONTRIBUTING document.
• I have performed a self-review of my code.
• I have added tests that prove my fix is effective or that my feature works.
• This change requires a documentation update.

Description

• It fixes !41011
• Here is a brief description of what this PR does

Objects are systematically locked even when no one else is using them (only one user is using GLPI).

[cconard96]

I think I am missing something here. Where is the "users_id" field being set for the lock?
ObjectLock::lockObject is only called by ObjectLock::manageObjectLock which is only called statically by CommonGLPI::display. The itemtype and items_id fields seem set, but not users_id.

[trasher]

I think I am missing something here. Where is the "users_id" field being set for the lock? ObjectLock::lockObject is only called by ObjectLock::manageObjectLock which is only called statically by CommonGLPI::display. The itemtype and items_id fields seem set, but not users_id.

I do not know well this part; but it seems users_id is set line 142.

I do not understand is this issue comes from; there is no recent change on this file (or this is a very old issue?)

[cedric-anne]

I do not understand is this issue comes from; there is no recent change on this file (or this is a very old issue?)

It has been refactored in GLPI 11.0, see 6037279

[trasher]

I do not understand is this issue comes from; there is no recent change on this file (or this is a very old issue?)

It has been refactored in GLPI 11.0, see 6037279

Yeah, that's quite old... I had in mind I've tested objectlock a few weeks ago and did not have that issue - but maybe I'm wrong, or I tested another branch...

Anyway, I can reproduce the issue; and the fix does not solve it :/

• lock activated on Computer,
• logged in as glpi user,
• open a Computer - no lock
• f5 => computer is locked.

[cconard96]

I do not know well this part; but it seems users_id is set line 142.

Yes, but that is after the condition the "else" was added to.

IMO this feature has always had a little unreliability to it. In my testing, I see the fetch call to unlockobject.php get cancelled sometimes during a refresh (possibly based on if there is user interaction before the reload), the new page request starts, and then the "fallback" jQuery AJAX call gets sent after the new page request finishes.
The expected way to send data before leaving the page is with sendBeacon but this has also been discussed before and has potential issues (#5828 (comment)).

I don't have a recommendation here at the moment. Obviously a SPA would have more control over the lifecycle of a "page" but that's not a feasible discussion for a while.

[trasher]

So my problem was different, and solved by last commit.

I do not know how to reproduce initial issue :/

[Rom1-B]

The customer's issue was the same as the one you described:

• lock activated on Computer,
• logged in as glpi user,
• open a Computer - lock

[trasher]

The customer's issue was the same as the one you described:
[...]

Not exactly, on your screenshot the message is "Read-only mode"; which is another case in the template file templates/layout/parts/objectlock_message.html.twig :(

[Rom1-B]

At the client’s end, we see the message “Locked by...” (!41011 --> I did not share the screenshot to avoid exposing names). On my local instance, the message displayed is “Read-only mode.” In both cases, no other user is active on the object (or even in GLPI). However, GLPI still considers the object as locked, either by the user themselves (which should not block access) or by no one, indicating a potential issue with the lock declaration.

[cconard96]

At the client’s end, we see the message “Locked by...” (!41011 --> I did not share the screenshot to avoid exposing names). On my local instance, the message displayed is “Read-only mode.” In both cases, no other user is active on the object (or even in GLPI). However, GLPI still considers the object as locked, either by the user themselves (which should not block access) or by no one, indicating a potential issue with the lock declaration.

Not sure the reason but having the message that an object is locked by you is expected in some cases.

Are you sure the object is actually still locked though rather that what I described with the unlock request happening after the browser requests the page?

[Rom1-B]

Not sure the reason but having the message that an object is locked by you is expected in some cases.

Which ones?

Honestly, the mechanics of locks are obscure, and the desired behavior is no clearer to me.

[cconard96]

Not sure the reason but having the message that an object is locked by you is expected in some cases.

Which ones?

Honestly, the mechanics of locks are obscure, and the desired behavior is no clearer to me.

Your guess is as good as mine but the functionality has existed as long as object locks themselves which means there was some intention there.
https://github.com/glpi-project/glpi/pull/363/files#diff-198adf54a62ba5a28b7f706711e0ea8caac4bca4a90ea14dd174ebeb9c6b4588

[cconard96]

The feature is probably due for a complete redesign

[Rom1-B]

In my view, the object lock is designed to prevent two people from editing the same item simultaneously, which could result in one user’s changes overwriting the other’s. If I am the owner of the lock, there is no valid reason for me to be blocked by myself. This appears to be a bug that has been present for many years.

[trasher]

Isn't that to prevent usage in several browsers tab?
Anyway, I cannot tell more - I do not know how it should work; this feature was present before I even heard of GLPI :D

[cconard96]

I was only using it at the time, not developing 🤷‍♂️.

I messaged the original author to try to get their thoughts on it.

[AdrienClairembault]

In my view, the object lock is designed to prevent two people from editing the same item simultaneously, which could result in one user’s changes overwriting the other’s. If I am the owner of the lock, there is no valid reason for me to be blocked by myself.

I agree with that👍

[cconard96]

Isn't that to prevent usage in several browsers tab?

@tomolimo said that the object locked by yourself message was indeed for that reason. If you have the item open in multiple tabs, windows or even devices (maybe you start editing something from a phone and then finish when returning to your desk?), it is to prevent potential conflicts.

[cconard96]

I'm currently thinking that there is still no real short-term solution here to make the unlock request reliable when reloading/navigating away. These kinds of requests inside the beforeunload event handlers are meant to be small, low priority and not block navigation.

I cannot think of a way to have the object lock check done/redone after the initial page render either without redoing how it is handled on the server as the session is modified if the item is locked when it is loaded which affects the form UI like hiding the edit button.

[orthagh]

If you have the item open in multiple tabs, windows or even devices (maybe you start editing something from a phone and then finish when returning to your desk?), it is to prevent potential conflicts.

I confirm that, as it was a feature requested initially

About the issue, I'm not able to replicate it systematically
Sometime after a Ctrl+r, I lock myself, sometime not, I guess due to race condition.

But I can't replicate the lock on initial view of a computer, I still need to force refresh the page to have the issue.

The current fix seems to remove the feature of multiple tabs open, and I don't know if it's widely used.
Probably not, and so, I'm not against remove it.
I guess the beforeunload async xhr will still be present sometimes, even in cases of multiples users.

About a full rework of the feature, I'm not convinced, as we can probably use our time on something more important.

[Rom1-B]

For information, the customer has tested and approved the patch.