Story #2608

Implement a websocket read-only event bus backed by the logs table

Added by Tom Clegg over 7 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
04/17/2014
Due date:
% Done:

100%

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

Subtasks

Task #2615: Write testsResolvedPeter Amstutz

Task #2611: Research websockets support in railsResolvedPeter Amstutz

Task #2693: Review origin-2608-websocket-event-bus-alt2ResolvedTim Pierce

Task #2616: Support filtering what events to receiveResolvedPeter Amstutz

Task #2619: Screenshots/demoResolvedPeter Amstutz

Task #2613: Apply permissions as user with API tokenResolvedPeter Amstutz

Task #2750: Websockets working in deploymentResolvedWard Vandewege

History

#1 Updated by Peter Amstutz over 7 years ago

  • Story points changed from 2.0 to 3.0

#2 Updated by Peter Amstutz over 7 years ago

  • Assigned To set to Peter Amstutz
  • Story points changed from 3.0 to 4.0

#3 Updated by Tim Pierce over 7 years ago

Code review comments. This is a first pass, I'll review more in-depth later, but this might help to start.

Method summary comments that would be useful:
  • EventBus.push_events
  • EventBus.on_connect
  • RackSocket.initialize
  • RackSocket.call
  • RecordFilters.record_filters

We should also have module comment docs for EventBus, RackSocket, LoadParam, RecordFilters etc. In general I'd like to see module comments that describe briefly what the module is for and what subsystem it supports, and method summary comments on every nontrivial method. Doesn't have to go into a huge amount of detail, just summarize what the method does, what it returns and what arguments it takes.

  • services/api/lib/eventbus.rb
    • in alloc_filter_id: what are those parentheses doing? Does that actually serve a function?
  • services/api/config/initializers/eventbus.rb
    • Recommend just checking if ENV['ARVADOS_WEBSOCKETS'] and not testing for a specific value (this is what we do in the client with ARVADOS_API_HOST_INSECURE)
  • services/api/db/migrate/20140423132913_add_object_owner_to_logs.rb
    • Are :new_attributes or :old_attributes set somewhere else in the migration? I don't see them mentioned anywhere else in the code. Or is this Rails reflection magic?

#4 Updated by Tim Pierce over 7 years ago

More code review:

  • services/api/config/application.default.yml
    • Do we need to provide people with guidance on setting websocket_address properly? Or can we set it to a sensible default? (I'm assuming wss://localhost:3002/websocket will not be a useful setting in production -- if it is, I'm misunderstanding something badly)
  • services/api/app/middlewares/arvados_api_token.rb
    • I'm new to the use of "middlewares" here. After reading some of the Rack documentation, it sounds like the idea is: when a request comes in, it's run through each of the config.middleware classes in turn, running the call method on the request and forwarding the resulting request to the next middleware. Is that about right?
    • In turn, is that why ArvadosApiToken.call needs to clean up each time it's called -- because one ArvadosApiToken object in the middleware stack may be used to process many requests from different users?
    • Is it meaningful for an ArvadosApiToken.call to be invoked when @app is nil? Should that trigger an error or warning of some kind?
  • services/api/app/models/arvados_model.rb
    • Per discussion in IRC, suggest moving the user.is_admin check outside the SQL query entirely (both to make the code clearer and possibly to short-circuit the need to do a query at all)

#5 Updated by Tim Pierce over 7 years ago

  • services/api/lib/eventbus.rb
    • I'm concerned that for the websocket to have its own user property updated from current_user is asking for trouble when ws.user doesn't get updated or cleared at the right time. Is this a necessary consequence of WebsocketController being subclassed from ApplicationController or something like that?
    • Should the EventBus.on_connect background thread reset @bgthread before it terminates? Right now it appears that nothing will ever set @bgthread to false once this thread has been started.
  • services/api/test/integration/websocket_test.rb
    • Nice selection of tests, very thorough.
    • Does EventMachine guarantee that events will be emitted in the order they were received? If not, we should make sure that tests which generate multiple events can handle the results out-of-order (relevant to test "filter events", "multiple filters", etc)
    • test "connect, subscribe, filter events" should assert that there are no more events waiting to be processed.
    • test "connect, subscribe, multiple filters" should include at least one event not captured by any filter, and should assert that there are no more events waiting to be processed.
    • test "connect, subscribe, get event, unsubscribe": why does this test have a timer to close the websocket after 3 seconds?
    • test "connect, subscribe, get event, try to unsubscribe with bogus filter_id": just to be pedantic, let's ensure that the filter_id we use to unsubscribe is different from the one that was created -- e.g. bogus_filter_id = d["filter_id"] + 1
    • test "connect, try subscribe too many filters": I hate magic constants. Can we replace "16" with EventBus.MAX_FILTERS here?

#6 Updated by Peter Amstutz over 7 years ago

  • Assigned To changed from Peter Amstutz to Tim Pierce
  1. Added lots of code comments
  2. Removed filter ids in favor of specifying the filter to remove directly.
  3. new_attribute and old_attribute are part of the logs table from before, the migration is just extracting a from those hash values and putting it into a real queryable column
  4. Added comments to application.yml.example
  5. Yep, that's how Rack works
  6. The functional tests now use ArvadosApiToken to set the current user environment, in that situation it doesn't really make sense to pass control on to another app. See test_helper.rb#authorize_with
  7. Added is_admin check to #readable_by
  8. current_user is only valid during connection setup, because we know ArvadosApiToken has already run to set up the environment, so ws.user is set to record which user is associated that websocket connection. The message, close, and channel events are processed on the EventMachine thread, which is separate from the Rack request thread and shared by all websocket connections, so we don't have the current_user environment set up.
  9. EventMachine is a single-threaded event queue architecture, so events are definitely processed in the order they are received.
  10. Added guard states to various test to assert no more events are processed
  11. Added comment about the timer
  12. Changed how subscribing works
  13. No more magic constants.

#7 Updated by Tim Pierce over 7 years ago

  • Assigned To changed from Tim Pierce to Peter Amstutz

LGTM

#8 Updated by Peter Amstutz over 7 years ago

  • Status changed from New to Resolved
  • Story points deleted (4.0)

Also available in: Atom PDF