search

nextcloud / files_lock

GNU Affero General Public License v3.0
24 stars 8 forks source link

with files client automated locking we need a sane default value #164

Closed mgallien closed 11 months ago

mgallien commented 1 year ago

would prevent having locks with 0 tiemout because the config value is never changed on server

there will be more locks since desktop files client will automatically lock office files making this more visible

mgallien commented 1 year ago

discussed with @allexzander following tests of the release candidate of desktop client next feature release

mgallien commented 1 year ago

related change on desktop files client https://github.com/nextcloud/desktop/pull/6027

juliushaertl commented 1 year ago

I tend to agree that having a somewhat reasonable lock timeout is better, especially if automatic locking on clients happens more often now. We used to have this in the past but switched to the infinite one per discussion in https://github.com/nextcloud/files_lock/issues/49.

One issue with the default timeout is that if users intentionally lock because they go on a longer flight for example, then the lock would just timeout and no longer prevent other users from editing. However this seems more like and edge case compared to day to day usage and is still safeguarded by versions that one could restore to get changes back. Also admins might still be able to unlock anyways. There should also be a conflict dialog anyways so all predictable behaviour.

Fine with me but involving @AndyScherzinger and @tobiasKaminsky as involved in the initial decision.

mgallien commented 1 year ago

I tend to agree that having a somewhat reasonable lock timeout is better, especially if automatic locking on clients happens more often now. We used to have this in the past but switched to the infinite one per discussion in #49.

One issue with the default timeout is that if users intentionally lock because they go on a longer flight for example, then the lock would just timeout and no longer prevent other users from editing. However this seems more like and edge case compared to day to day usage and is still safeguarded by versions that one could restore to get changes back. Also admins might still be able to unlock anyways. There should also be a conflict dialog anyways so all predictable behaviour.

Fine with me but involving @AndyScherzinger and @tobiasKaminsky as involved in the initial decision.

The fact that 0 means infinite was definitely a misunderstanding from us desktop files client team We always assumed that 0 meant 0 We may also adjust our code a little bit more to have proper handling of the 0 value @tobiasKaminsky ?

mgallien commented 1 year ago

another related fix for desktop client https://github.com/nextcloud/desktop/pull/6059

AndyScherzinger commented 1 year ago

Can you give me some additional context on why you would change this away from infinite? That just works around the problem of improper unlocking or at least that is how it sounds to me but I might be completely wrong here.

Also checking the referenced decision, I expect:

mgallien commented 11 months ago

assuming that the main reason for this PR was a misunderstanding over the use of 0 as an infinite timeout, I would like to close this one in favor of https://github.com/nextcloud/files_lock/pull/175 (just improving the API doc around the lock-timeout)

mgallien commented 11 months ago

Can you give me some additional context on why you would change this away from infinite? That just works around the problem of improper unlocking or at least that is how it sounds to me but I might be completely wrong here.

Also checking the referenced decision, I expect:

* clients aren't handling server-unlock via timeouts (yet)

desktop client will schedule a server sync font expired locks to ensure we do not miss files being unlock after the timeout expired that was not properly handling the 0 value

but as I said, I am all for closing this one in favor of doc improvements