aetcd icon indicating copy to clipboard operation
aetcd copied to clipboard

Raise RuntimeError if watcher.cancel() is called after client.close()

Open mk-fg opened this issue 3 years ago • 2 comments

Currently client.watch() will return a watcher object that can be used separately from the client. Current behavior when watcher.close() is called after client.close() - AttributeError: 'NoneType' object has no attribute 'cancel' - is confusing and unclear.

I think two ways to handle this better are:

  • Make watcher.cancel() a no-op after client is closed. This mimics how async for kv in watcher: iterator currently works.

  • Have watcher.cancel() raise a clear "must not be called when client is already closed" exception.

This PR does the latter, assuming that it is a programming error to call watcher.cancel() for closed client. In my use-case closing the client should cancel all watcher iterators already, so such call indeed indicates some code bug. Picked RuntimeError because it's used for same types of bugs in asyncio itself and also in client.py already, and there should never be a need to handle it in any way (i.e. handling RuntimeError is almost certainly a bug in the code).

Alternative viewpoint on the API can be that w = client.watch(); try: ... finally: w.cancel() pattern should be supported, in which case .cancel() should be a no-op (to avoid raising exception in "finally"). If this is the case, rtypes.Watch should also support aenter/aexit to work in a more conventional with client.watch() as w: ... pattern. Assuming that this is not implemented on purpose, RuntimeError variant above seem to be a more fitting option.

mk-fg avatar Jan 14 '22 21:01 mk-fg

This mimics how "async for kv in watcher:" iterator currently works.

If trying to do something with watcher after client is already closed via watcher.cancel() should be considered a bug, then I think opening any new iterators on it should almost certainly be a bug as well. Current behavior (as mentioned) is that such iterators simply don't produce any values, but I've also added a check to raise RuntimeError for trying to open new iterators on such bogus watcher objects in later force-push.

mk-fg avatar Jan 14 '22 21:01 mk-fg

Haven't thought of it at first, but there's also one behavior around client.close() that kinda bothers me, indicated by # key=2 event will be lost on close(), if not consumed already in the test with last rebase - if there are consumers of events on async watcher iterators, they will be in a race with client.close().

I.e. if event gets received from etcd, followed by closing the client, iterators can get the event or not, somewhat randomly, depending on whether consumer or close() gets scheduled first.

I think this will lead to inconsistent behavior in apps, and potential bugs, if a dev would expect this to go one specific way or another. But it seem to be out of scope for this PR, maybe will fix in another one. Just thought that it was worth mentioning, at least.

mk-fg avatar Jan 18 '22 00:01 mk-fg