crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.36k stars 1.62k forks source link

Non-blocking `flock` without raising #12722

Open straight-shoota opened 1 year ago

straight-shoota commented 1 year ago

flock methods on FileDescriptor have two modes of operation: blocking and non-blocking. In the first case, the method blocks until it gets the lock (see #12721 for a proposal to make that fiber-aware). In the second case, the method raises IO::Error when the lock could not be attained immediately.

I don't think the raising behaviour is very useful. The non-blocking variant is typically used to try to get a lock and if it doesn't work, do something else. That may be an error, but the application could just do something else instead. For that something else, you need to rescue the exception and identify that it's EWOULBLOCK (and not some other actual error) in order to continue with "do something else instead".

This is quite some extra effort at the call site and most importantly, the entire exception raising is unnecessary.

Actually, this sounds more like job for a Bool return value to me: true indicates the lock was gained, false that it didn't. The method would only raise for an actual error (such as EINVAL).

I propose to introduce these two method for that:

And IMO this makes the raising semantics of FileDescriptor#flock_exclusive(blocking: false) and FileDescriptor#flock_shared(blocking: false) unnecessary. So I would also propose to deprecate these methods with an argument (the default value for blocking is true, so the argless method continue to exist).

straight-shoota commented 1 year ago

Maybe the failing variants should be kept around. I think there could be good use cases where you expect to be able to attain the lock and in case of failure you expect a proper exception raised (instead of having to do that yourself).

I would consider to extract this semantic to separate methods with a bang, though. This would communicate the different behaviours more clearly:

# blocks until the lock is attained
def flock_exclusive
# raises if the block can't be attained
def flock_exclusive!
# returns `false` if the block can't be attained
def flock_exclusive?

The same schema would apply to flock_shared. And I suppose all methods would have yielding variants with the same semantics, where the question method only yields if the block was attained.

straight-shoota commented 1 year ago

The suggestion of introducing a timeout (https://github.com/crystal-lang/crystal/issues/12721#issuecomment-1308276097) would also be a relevant consideration for this API change.

The variants described in the previous comment only account for indefinite blocking, in which case there is no distinction between raising/non-raising. But when there's a timeout, it's relevant what happens when the timeout triggers.

The timeout should probably apply to both, raising and non-raising variants, which means there would be four variants in total.

For this we can use two method names (? suffix for non-raising) and the timeout parameter to determine whether it's blocking and for how long.

# raises if the lock can't be attained within *timeout*
def flock_exclusive(timeout : Time::Span = Time::Span::MAX)
# returns `false` if the lock can't be attained within *timeout*
def flock_exclusive?(timeout : Time::Span = Time::Span::MAX)

Non-blocking behaviour can be achieved with timeout: 0.seconds (equivalent to blocking: false in the current API).