Locks unlimited in time have undefined behaviour

[jbarnoud]

State locks have all the facilities to handle a lock without timeout (for instance here). Such a permanent lock would occur if the timeout is set to None, see

nanover-protocol/python-libraries/nanover-core/src/nanover/state/state_service.py

Line 217 in 6b7f45b

if duration is not None:

. However, the protocol does not allow the timeout to be None as a None timeout in the lock update request would remove the lock (See the protocol definition and its implementation).

Do we want to keep this behaviour in the code or match the protocol more closely? Keeping the code as is may allow a server to create unlimited locks, which may be desirable, but it could lead to unreachable code and cause devs to misinterpret the expected behaviour, which is bad.

[jbarnoud]

Note that the rust server has the same behaviour as the python one: both would allow unlimited locks but it is unreachable in both servers.

EDIT: Here is the issue on the rust server IRL2/nanover-rs#216

[jbarnoud]

Pinging @Ragzouken for an opinion.

[Ragzouken]

First instinct is that unlimited locks are a feature of KeyLockableMap that we simply don't use/expose for state service.

As far as I can tell there aren't actually any tests for lock expiration for either KeyLockableMap or state service. I think expiry was added later to deal with the issue of clients exiting while moving the box.

The code for non-expiring locks is trivial to remove and later restore, so it wouldn't hurt to remove it if that seems sensible. Maybe in the future we would like to support client's creating non-expiring locks when we have the lifecycle stuff to expire them on client exit. Maybe locks is something that gets a full review when state lifecycle stuff comes around anyway.

No strong opinion.

[jbarnoud]

First instinct is that unlimited locks are a feature of KeyLockableMap that we simply don't use/expose for state service.

I am happy to close the issue from this answer. I let you choose to close it or not, in case you'd like to keep it open for when you look at the life cycle.