Bug #9135

[SDKs] `arv ws -j` exits immediately with an error

Added by Brett Smith over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Brett Smith
Category:
SDKs
Target version:
Start date:
05/06/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-

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.


Subtasks

Task #9157: Review 9135-eventclient-run-forever-wipResolvedBrett Smith


Related issues

Related to Arvados - Story #7658: [SDK] Python websockets reconnect on unexpected closeResolved04/04/2016

Precedes (1 day) Arvados - Story #9217: [SDKs] Define and document Python events interfaceNew05/09/201605/09/2016

Associated revisions

Revision 7f7ed633
Added by Brett Smith over 5 years ago

Merge branch '9135-eventclient-run-forever-wip'

Closes #9135, #9157.

History

#1 Updated by Brett Smith over 5 years ago

  • Description updated (diff)

#2 Updated by Brett Smith over 5 years ago

Per discussion in IRC, the public interface of EventClient accidentally changed in #7658. For the sake of other clients, that interface needs to be restored. This means that EventClient needs to expose whatever public methods it had prior to #7658, including those inherited from WebSocketClient.

#3 Updated by Brett Smith over 5 years ago

  • Target version set to Arvados Future Sprints

#4 Updated by Brett Smith over 5 years ago

  • Status changed from New to In Progress
  • Assigned To set to Brett Smith

#5 Updated by Brett Smith over 5 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'])

#6 Updated by Brett Smith over 5 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.

#7 Updated by Peter Amstutz over 5 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.

#8 Updated by Brett Smith over 5 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.

#9 Updated by Brett Smith over 5 years ago

  • Target version changed from 2016-05-11 sprint to Arvados Future Sprints

#10 Updated by Brett Smith over 5 years ago

  • Target version changed from Arvados Future Sprints to 2016-05-11 sprint

#11 Updated by Peter Amstutz over 5 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.

#12 Updated by Brett Smith over 5 years ago

  • Target version changed from 2016-05-11 sprint to 2016-05-25 sprint

#13 Updated by Brett Smith over 5 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.

#14 Updated by Peter Amstutz over 5 years ago

Thanks, LGTM @ db2aa9f

#15 Updated by Brett Smith over 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:7f7ed63314bec77bf93edc233514c50a1030cac4.

#16 Updated by Brett Smith over 5 years ago

#9217 is the follow-up ticket to do the rest of the work to clean up the interfaces here.

Also available in: Atom PDF