Bug #9135
closed[SDKs] `arv ws -j` exits immediately with an error
Description
After installing from master at bd8aa1d,
% arv-ws -j qr1hi-8i9sb-fsumu14u53co3yo 2016-05-04 09:20:06 arvados.arv-ws[25013] ERROR: 'EventClient' object has no attribute 'run_forever'
and then it exits 0 (!).
It works with ARVADOS_DISABLE_WEBSOCKETS=1
, so apparently this is a problem somewhere at the boundary between ws.py and the Websockets event class.
Related issues
Updated by Brett Smith over 8 years ago
Updated by Brett Smith over 8 years ago
- Target version set to Arvados Future Sprints
Updated by Brett Smith over 8 years ago
- Status changed from New to In Progress
- Assigned To set to Brett Smith
Updated by Brett Smith over 8 years ago
I think we should implement things that we previously inherited from WebSocketClient that are also implemented in PollClient. Presumably users are prepared to use either one, so it's less pressing to implement methods that wouldn't be available in PollClient anyway.
>>> import arvados.events as ae >>> pc_methods = set(name for name in dir(ae.PollClient) if not name.startswith("_")) >>> ws_methods = set(name for name in dir(ae.WebSocketClient) if not name.startswith("_")) >>> ec_methods = set(name for name in dir(ae.EventClient) if not name.startswith("_")) >>> missing_pc = pc_methods.difference(ec_methods) >>> missing_ws = ws_methods.difference(ec_methods) >>> missing_pc.intersection(missing_ws) set(['run_forever', 'daemon', 'run'])
Updated by Brett Smith over 8 years ago
Upon reflection, I'm not sure my last comment is right. It feels unclear what the right thing to do is, because it's not clear what the API is "supposed" to be here.
The main way to get an events client is to call arvados.events.subscribe()
. This will give you an EventClient (using Websockets) if it can and is allowed; or else it will give you a PollClient. If this is the way you're supposed to get an events client, then the API is the intersection of methods common to both classes. Those are subscribe, unsubscribe, run_forever, and close. (They both have a run method, too, but those methods have completely different signatures and semantics, so you can't safely call it on whatever subscribe() gives you back.)
But of course, there's nothing stopping clients from instantiating an EventClient directly--and if they want to ensure they get a Websockets connection, and don't fall back to polling, that's the only option available to them. If users do that, then we should err toward making all of the original EventClient methods available, including the ones inherited from WebSocketClient.
Either way, we have to add the run_forever method. That's easy enough. But it's less obvious to me whether the branch should make other methods available as well, or if it would actually be better to try to expose fewer methods in EventClient to enforce the idea that the proper interface is the small subset I listed above.
Updated by Peter Amstutz over 8 years ago
The intended API is that you use the module level subscribe()
to get back an Event object, the public API of the event object is subscribe(), unsubscribe(), close(), and run_forever()
(the last one should probably be renamed wait_for_close()
). I would be fine with making everything else in module be private.
Updated by Brett Smith over 8 years ago
- Target version changed from Arvados Future Sprints to 2016-05-11 sprint
9135-eventclient-run-forever-wip is up for review. In the interest of getting the critical bug fixed, the branch is neutral on the API question: it doesn't add other methods from WebSocketClient, but it doesn't mark any methods as private either.
Updated by Brett Smith over 8 years ago
- Target version changed from 2016-05-11 sprint to Arvados Future Sprints
Updated by Brett Smith over 8 years ago
- Target version changed from Arvados Future Sprints to 2016-05-11 sprint
Updated by Peter Amstutz over 8 years ago
The intended behavior of run_forever()
is to run until the socket is explicitly closed, however by delegating run_forever to the underlying Events object, it will only block until the current session terminates (despite the fact that we reconnect on unexpected close). Similarly, it doesn't make sense to delegate connect() and close_connection() for the same reason (no user of EventClient should be calling those methods).
Suggest that EventClient adopts the same approach as PollClient and uses an event variable to block until explicitly closed.
Updated by Brett Smith over 8 years ago
- Target version changed from 2016-05-11 sprint to 2016-05-25 sprint
Updated by Brett Smith over 8 years ago
Peter Amstutz wrote:
Similarly, it doesn't make sense to delegate connect() and close_connection() for the same reason (no user of EventClient should be calling those methods).
Per note-8, I was just preserving the existing behavior: these methods are delegated in master. But it's done.
Suggest that EventClient adopts the same approach as PollClient and uses an event variable to block until explicitly closed.
Yes, thanks. Done in db2aa9f.
Updated by Brett Smith over 8 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:7f7ed63314bec77bf93edc233514c50a1030cac4.
Updated by Brett Smith over 8 years ago
#9217 is the follow-up ticket to do the rest of the work to clean up the interfaces here.