Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mk-fg
Copy link
Contributor

@mk-fg mk-fg commented Jan 14, 2022

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 mk-fg force-pushed the nicer-error-on-watcher-cancel branch 3 times, most recently from 19bf4b2 to 20f45a6 Compare January 14, 2022 21:31
@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 14, 2022

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 mk-fg force-pushed the nicer-error-on-watcher-cancel branch 3 times, most recently from be1ff05 to 41ed34f Compare January 17, 2022 23:57
@mk-fg mk-fg force-pushed the nicer-error-on-watcher-cancel branch from 41ed34f to 122265c Compare January 18, 2022 00:10
@mk-fg
Copy link
Contributor Author

mk-fg commented Jan 18, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant