Arvados

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

• 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?

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)

• 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?