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

ActionCable protocol negotiation #24224

Merged
merged 1 commit into from Apr 5, 2016

Conversation

Projects
None yet
9 participants
@danielrhodes
Contributor

danielrhodes commented Mar 17, 2016

A first stab at negotiating an acceptable protocol as was mentioned in #23976 @maclover7 @jeremy @javan

Going forward, if the ActionCable protocol changes, there ought to be some way for the client/server to negotiate this change and gracefully stop communicating if no common protocol can be established.

Using the Sec-Websocket-Protocol from the WebSocket spec, the client and server negotiate a protocol from a list. Most of the work is being done by the WebSocket driver and the web browser WebSocket client, so all this really does is make sure the client does not get into a bad state if the server has been upgraded and the client has not.

The browser websocket client automatically chooses to disconnect if the server does not respond with an acceptable protocol (this is not configurable), so instead of trying to reconnect (as this would not be useful), the client shuts down the monitor.

@rails-bot

This comment has been minimized.

rails-bot commented Mar 17, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@danielrhodes danielrhodes force-pushed the danielrhodes:actioncable-websocket-protocols branch 3 times, most recently Mar 17, 2016

@javan

This comment has been minimized.

Member

javan commented Mar 17, 2016

What browsers have you tested this in? I'm curious if protocols and onclose event codes are broadly supported. Unclear from documentation I've found online: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket#Browser_compatibility and https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent#Browser_compatibility

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -46,6 +46,9 @@ class ActionCable.Connection
else
@open()
protocol: ->
return @webSocket?.protocol

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

return is implicit in CoffeeScript, you can cut it.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
ActionCable.log("WebSocket onclose event")
if event.code == 1006
ActionCable.log("WebSocket received abnormal close error (" + event.code + "): " + event.reason)

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

More CoffeeScripty:

if event.code is 1006
  ActionCable.log("WebSocket received abnormal close error (#{event.code}):", event.reason)

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

Also, why an additional log for 1006 only? Might as well log event.code, event.reason, and event.wasClean with every close.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
if event.code == 1006
ActionCable.log("WebSocket received abnormal close error (" + event.code + "): " + event.reason)
if !event.wasClean

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

In CoffeeScript you can do unless event.wasClean (or if not event.wasClean).

@jeremy

This comment has been minimized.

Member

jeremy commented Mar 17, 2016

Cool! TIL that Sec-Websocket-Protocol is for subprotocol negotiation.

@jeremy

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
if !event.wasClean
ActionCable.log("WebSocket says close was unclean. Stopping ConnectionMonitor.")
@monitor.stop()

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Can we bubble this up to the app in some way so it can react in the UI, e.g. show a prompt to reload the page or auto-reload it?

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

Yeah we need a way of telling the app why there was a hard disconnect. To be discussed, as there are other cases where we need this as well (e.g. authentication failure).

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Mar 17, 2016

@javan Ok I tested it on Chrome, Safari, Firefox, and Mobile Safari. I don't have IE, but it does have the CloseEvent. Mobile Safari was the only one which behaved differently from the others (it did not automatically disconnect when the negotiation failed). Therefore, I changed the code to check the protocol negotiated to see if it is in the list the client knows it can accept and if not, to disconnect. I likewise took out that close event code stuff and checked the protocols as a way to decide if the monitor should be stopped. I think that should resolve your comments.

@jeremy

View changes

actioncable/lib/action_cable.rb Outdated
# This protocol ought to be versioned separately
# from the rest of the Gem
PROTOCOLS = ["actioncable-protocol-1.0.0".freeze].freeze

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

How about e.g. actioncable-v17-json? Tack on the transport encoding (which is always JSON now) and use a big chunky version that looks nothing like the AC version number since the only purpose is to break major compat (there are no "minor" changes).

Then the server can negotiate by parsing out the requested version and transfer encoding and picking the matching protocol handler for the conn.

This comment has been minimized.

@matthewd

matthewd Mar 17, 2016

Member

Do we only want to version the ACa transport version, or also provide for an application-level value? I'm thinking of something equivalent to config.assets.version...

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

@jeremy This is a good idea.

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

@matthewd This kind of gets to a bigger question of how much backwards-compatibility we build into this.

If somebody wants to make their ActionCable server a publicly available resource, then backwards compatibility becomes important. Likewise, I made a Swift client for ActionCable and a changing protocol version becomes a real pain for the developer because the longer upgrade cycle becomes a chicken-and-egg problem.

On the other hand, if this is really only used so a person can communicate with their own backend in the browser, there doesn't need to be a lot of backwards compatibility built-in. Only enough so that the client can gracefully degrade until the next reload.

In either case, there is no benefit for the developer in being able to pin a version because if the server is able to support a version, it should because it's an internal protocol.

I'm interested to see what feedback we get about this.

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

App-level protocol versioning is interesting. It could be helpful, at minimum, to provide a big hammer so apps can wiggle out of a stale-client compatibility mess by bumping the protocol.

For other cases, like changing channel behavior, I'd lean to leaving it up to apps to make changes in a compatible way. But, heck, leaning on bumping the app proto version could be simpler in many of these cases too.

@jeremy

View changes

actioncable/lib/action_cable/connection/base.rb Outdated
@websocket = ActionCable::Connection::WebSocket.new(env, self, event_loop, server.config.client_socket_class)
@options = {:protocols => ActionCable::PROTOCOLS}
@websocket = ActionCable::Connection::WebSocket.new(@env, @options, self, event_loop, server.config.client_socket_class)

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Good one to pass as a keyword arg, no need to keep @options around

@jeremy

View changes

actioncable/lib/action_cable/connection/client_socket.rb Outdated
@@ -29,10 +29,11 @@ def self.secure_request?(env)
attr_reader :env, :url
def initialize(env, event_target, event_loop)
def initialize(env, options, event_target, event_loop)

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member
def initialize(env, options, event_target, event_loop, protocols: ActionCable::PROTOCOLS)

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Or glob the keyword args as websocket options:

def initialize(env, options, event_target, event_loop, **websocket_options)
@driver = ::WebSocket::Driver.rack(self, @websocket_options)

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

Would be happy to do this as it is cleaner. I haven't seen much use of the Ruby keyword arguments in the Rails codebase, but I guess it's ok now?

UPDATE: Seems Ruby 2+ is now required for Rails 5, so I'll do the keyword args.

@jeremy

View changes

actioncable/lib/action_cable/connection/client_socket.rb Outdated
@@ -42,7 +43,7 @@ def initialize(env, event_target, event_loop)
@ready_state = CONNECTING
# The driver calls +env+, +url+, and +write+
@driver = ::WebSocket::Driver.rack(self)
@driver = ::WebSocket::Driver.rack(self, @options)

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member
@driver = ::WebSocket::Driver.rack(self, protocols: protocols)
@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Mar 17, 2016

In the comments @jeremy pointed out that it would be helpful if there was some way the client could communicate to the app a reason for the disconnect. There isn't currently a global way of doing this, so any opinions on this? We can pass some sort of reason object through to the disconnect method like WebSocket does.

@jeremy

View changes

actioncable/lib/action_cable/connection/faye_client_socket.rb Outdated
@env = env
@event_target = event_target
@options = options || {}

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Ditto

@jeremy

View changes

actioncable/lib/action_cable/connection/faye_client_socket.rb Outdated
@faye = Faye::WebSocket.new(@env)
protocols = @options[:protocols] || nil
@faye = Faye::WebSocket.new(@env, protocols)

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Do we need the || nil? Would only kick in if protocols was oddly false

@jeremy

View changes

actioncable/lib/action_cable/connection/web_socket.rb Outdated
def initialize(env, event_target, event_loop, client_socket_class)
@websocket = ::WebSocket::Driver.websocket?(env) ? client_socket_class.new(env, event_target, event_loop) : nil
def initialize(env, options, event_target, event_loop, client_socket_class)
@websocket = ::WebSocket::Driver.websocket?(env) ? client_socket_class.new(env, options, event_target, event_loop) : nil
end

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Ditto

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
ActionCable.log("WebSocket onclose event")
if @webSocket?.protocol not in ActionCable.PROTOCOLS
ActionCable.log("WebSocket is closing because there is no common protocol. Stopping ConnectionMonitor.")
@monitor.stop()

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

This will prevent the connection monitor from infinitely attempting to reconnect (as noted in #23976 (comment)), but leaves you disconnected until you reload your browser. Think we should notify subscriptions of this state so they can manage / instruct reloading.

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

Yes, @jeremy wants something along these lines. I guess just a simple object with code and a reason passed into the disconnect method?

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

Yeah, I think that'd work fine. Would be cool to hot-reload the client-side js, but that's probably more trouble than it's worth for what will be an infrequent event.

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

I guess if you could plug into Sprockets somehow, you could pull a new version of the classes (perhaps namespaced differently for each asset build) and then hot swap in the ActionCable classes and reconnect. It would be super cool, but yes a lot of trouble.

As a side note: Facebook was doing something similar on Android for awhile, before Google made them stop by modifying the TOS for the Google Play Store.

@javan

View changes

actioncable/app/assets/javascripts/action_cable.coffee.erb Outdated
@@ -3,6 +3,7 @@
@ActionCable =
INTERNAL: <%= ActionCable::INTERNAL.to_json %>
PROTOCOLS: <%= ActionCable::PROTOCOLS.to_json %>

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

How about adding :protocols to INTERNAL instead? It's a general purpose data bag.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
# This is in the case that the browser client wants
# to open the connection regardless of whether a
# protocol was negotiated successfully.
if @webSocket?.protocol not in ActionCable.PROTOCOLS

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

I'd extract this to a new method since it's duplicated

@maclover7

View changes

actioncable/lib/action_cable.rb Outdated
# This protocol ought to be versioned separately
# from the rest of the Gem
PROTOCOLS = ["actioncable-v1-json".freeze].freeze

This comment has been minimized.

@maclover7

maclover7 Mar 17, 2016

Member

Do we want this to have the same version number as the gem? Would prevent having two different version numbers for Action Cable...

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

The protocol version is distinct from the library version and will be stable over longer periods. Using separate versions is intentional.

@jeremy

View changes

actioncable/lib/action_cable/connection/faye_client_socket.rb Outdated
@@ -31,7 +36,9 @@ def rack_response
private
def connect
return if @faye
@faye = Faye::WebSocket.new(@env)
protocols = @options[:protocols] || nil

This comment has been minimized.

@jeremy

jeremy Mar 17, 2016

Member

Missed this line? options gone now

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

Oops :-)

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
ActionCable.log("WebSocket onclose event")
if @webSocket?.protocol not in ActionCable.PROTOCOLS
ActionCable.log("WebSocket is closing because there is no common protocol. Stopping ConnectionMonitor.")

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

For debugging, it would be useful to log the client and server protocol versions rather than say they're not common.

This comment has been minimized.

@danielrhodes

danielrhodes Mar 17, 2016

Contributor

The server does not tell the client which subprotocols it supports. It simply chooses one or none, and then responds with that protocol in the header.

This comment has been minimized.

@javan

javan Mar 17, 2016

Member

I mean add @webSocket.protocol and ActionCable.PROTOCOLS to the log message

@danielrhodes danielrhodes force-pushed the danielrhodes:actioncable-websocket-protocols branch 3 times, most recently Mar 17, 2016

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Apr 2, 2016

What's the status of this? Are we good to merge? I think after the reviews, the footprint of the change in terms of behavior/code is now quite low.

@maclover7

This comment has been minimized.

Member

maclover7 commented Apr 2, 2016

@danielrhodes I think this is coming along nicely -- can you try to squash down some of your commits?

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Apr 2, 2016

Yes will do that now. Github actually just added a feature to do this automatically on merge -- something I'm sure you will appreciate!

@@ -35,7 +35,8 @@ module ActionCable
confirmation: 'confirm_subscription'.freeze,
rejection: 'reject_subscription'.freeze
},
default_mount_path: '/cable'.freeze
default_mount_path: '/cable'.freeze,
protocols: ["actioncable-v1-json".freeze, "actioncable-unsupported".freeze].freeze

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Apologies for asking again since I think you explained this already, but it's not clear to me what the purpose of actioncable-unsupported protocol is. Perhaps it could be defined as a separate constant that gets included in this array so it can be documented separately.

Additionally, now that we have a v1 protocol in place, what kinds of changes warrant bumping the version? (I know the answer, but think it's worth documenting).

This comment has been minimized.

@maclover7

maclover7 Apr 2, 2016

Member

Sorry if I've already asked this -- is there any downside to having the protocol version match the current Action Cable version? Any breaking changes with Action Cable's websockets communication infrastructure should theoretically also be a regular-Action Cable breaking change. Having two version numbers seems a bit much...

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

@javan Yeah the reason for this is that the browser side WebSocket client sticks with the bizarre recommended implementation where the client does not tell us why it disconnected, when it could either be a bad disconnect or a protocol mismatch. So the actioncable-unspported protocol is the way to get around this: it will always be the last to be chosen, and if it is, we know for sure there is a mismatch. The alternatives to this are really hacky.

@maclover7 I initially asked myself the same thing. However, the protocol version is distinct from the Rails version, and ideally it would be changed quite a bit less often than the Rails version. Also since these protocol changes are almost always breaking changes, semantic versioning is not quite appropriate here.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

protocols (Optional)
Either a single protocol string or an array of protocol strings. These strings are used to indicate sub-protocols, so that a single server can implement multiple WebSocket sub-protocols (for example, you might want one server to be able to handle different types of interactions depending on the specified protocol). If you don't specify a protocol string, an empty string is assumed.
-- https://developer.mozilla.org/en-US/docs/Web/API/WebSocket

Guess I'm still not clear why the client needs know about actioncable-unsupported. What would happen if we only pass the supported protocol and let the server disconnect if that protocol isn't specified?

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Sorry, that's not right, I know we're not disconnecting server side on protocol mismatch. But can the server set the protocol to actioncable-unsupported if the client doesn't open the WebSocket with a supported protocol?

For example, client connects with:

@webSocket = new WebSocket(@consumer.url, "actioncable-v1-json")

And if the server only knows about actioncable-v2-json then it responds by setting the protocol to actioncable-unsupported.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

The short answer is no. The server must choose one or none of the protocols the client presents to it. We provide a list of protocols to the Faye-Websocket driver, and we likewise give a list of supported protocols to the browser websocket client. If the server doesn't support any of the protocols, the "correct" way to handle this is to return nothing -- except we get penalized for doing this on the client side because of the implementation recommendations... hence this unsupported stuff.

The longer answer is that yes we could fork the Faye-Websocket driver and do this... but my experience with the browser websocket client has been that it is extremely stubborn, so it would probably not like this and throw an error if it received a protocol it did not send.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Gotcha. Thanks for explaining!

@javan

View changes

actioncable/app/assets/javascripts/action_cable/subscription.coffee Outdated
@@ -8,6 +8,12 @@
# connected: ->
# # Called once the subscription has been successfully completed
#
# disconnected: (event) ->

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Move away from dispatching an event here. It's a plain object now.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Are you saying name it something different? Any suggestions?

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Yeah, maybe document it as disconnected: ({ willAttemptReconnect: boolean }) ->?

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

I just don't want to suggest that it's an Event instance.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Yeah makes sense. I just changed it to that.

@danielrhodes danielrhodes force-pushed the danielrhodes:actioncable-websocket-protocols branch Apr 2, 2016

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@webSocket?.close()
close: ({allowReconnect} = {allowReconnect: true}) ->
@monitor.stop() unless allowReconnect
@webSocket?.close() unless @isClosed()

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Could do @webSocket?.close() if @isActive() and drop the isClosed method entirely. Side note: we intentionally named isActive to not match a connection state and isClosed doesn't follow that pattern.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Yep don't see why not. Changed.

ActionCable.log("WebSocket onclose event")
@disconnect()
return if @disconnected
@disconnected = true

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Tracking @disconnected was originally in place because to prevent calling the disconnect method twice since both the error and close handlers called it. Now that disconnect has been removed, I think we can rely on the connection state alone and remove @disconnected.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

I think that should stay for now. It still seems to prevent some conditions where onClose is called by the websocket even though the websocket is already disconnected, which I think would be confusing to the developer. Personally I like those callbacks to be very definitive: if disconnected is called, it means it really just disconnected. So we shouldn't be making lots of callbacks while we are trying to reestablish a connection in the background.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

👍 if that's the case. Strange that close would be dispatched twice though. Can you reproduce?

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Oh yeah for sure. On every unsuccessful connection this is how the browser says a connection did not happen. I guess it would have made more sense for them to add an event to onError, but what can you do?

@@ -2,7 +2,7 @@
# Encapsulate the cable connection held by the consumer. This is an internal class not intended for direct user manipulation.
{message_types} = ActionCable.INTERNAL
{message_types, protocols} = ActionCable.INTERNAL

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

How about splitting the protocols here into usefully named vars:

[supportedProtocols..., unsupportedProtocol] = protocols
@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -53,6 +58,9 @@ class ActionCable.Connection
@isState("open", "connecting")
# Private
isProtocolSupported: ->
[..., unsupported_protocol] = protocols
@getProtocol() in protocols and @getProtocol() isnt unsupported_protocol

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Using the suggested changes above, this would change to:

@getProtocol() in supportedProtocols

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

I had a back and forth with @kaspth about this... he wanted to minimize the heavy separation I made in the code between the supported and unsupported protocols. However, I don't think your suggestion conflicts with what he was looking for, so I made the change.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -74,10 +82,12 @@ class ActionCable.Connection
events:
message: (event) ->
return if @disconnected

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Is there a scenario where messages are received after calling webSocket.close()?

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Yes the server fires out the welcome message pretty fast after connecting. The server doesn't decide to disconnect on a protocol mismatch, the client does.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Can we prevent the server from sending messages on protocol mismatch?

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Yes we could. I ran through some scenarios where we could just tell the server to stop communicating in these cases, but it gets kind of murky and the websocket protocol seems to indicate the onus is on the client to disconnect when it doesn't like the protocol not the server. Also if somebody writes a client in something other than in the browser (that would be me :-P), they might not want that behavior. Specifically, the problem is that when a client is deployed in a mobile app, you want the client to be pretty flexible if the protocol needs to change -- so the server going dark is like the worst thing that could happen!

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Cool. Mostly asking out of curiosity. Adding this guard makes sense.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -53,6 +59,8 @@ class ActionCable.Connection
@isState("open", "connecting")
# Private
isProtocolSupported: ->
@getProtocol() isnt unsupportedProtocol and @getProtocol() in supportedProtocols

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Isn't checking @getProtocol() in supportedProtocols enough? If the protocol is in that array then it's not unsupportedProtocol.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Sure. It's just extra safety I guess.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -90,18 +100,18 @@ class ActionCable.Connection
open: ->
ActionCable.log("WebSocket onopen event")
@disconnected = false

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Should we set disconnected = false only if the protocol is supported and remove the disconnected check in the onMessage handler?

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

I tried this before, but that welcome message was coming in and making the connection monitor think it was still online. It doesn't happen when code is as it is.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member
open: ->
  ActionCable.log("WebSocket onopen event, protocol is '#{@getProtocol()}'")
  if @isProtocolSupported()
    @disconnected = false
  else
    ActionCable.log("WebSocket opened with an unsupported protocol. Stopping monitor and disconnecting.")
    @close(allowReconnect: false)

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Guess that ^ doesn't remove the need to check disconnected in the message handler, but I find it clearer since it consolidates the logging and removes the early return.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

Yeah I was fooling around with this and was going back and forth, but then I settled on a successful connection means actual server connection and a protocol is chosen. That work we did with the welcome message was actually quite useful in telling the client something was wrong during authentication failure, and we would lose this in the case where a successful connection means just connecting to the server.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

We've hit sporadic missing subscriptions in Basecamp. Hoping this is the cause! ❤️

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

I settled on a successful connection means actual server connection and a protocol is chosen.

👍 to that, but you're setting disconnected = false before checking the protocol.

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Reviewing the open handler and where it sets @disconnected is the only thing left to address before this can be merged, IMO.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 3, 2016

Contributor

Alright I changed the message handler to check @isActive instead. It works the same. I don't quite understand why it worked with @disconnected even though as you pointed out there could be a race condition -- whatever underlying threading is going on obviously prioritized the close before the message is all I can surmise.

This comment has been minimized.

@jeremy

jeremy Apr 4, 2016

Member

Re. subscription requests before welcome: we buffer incoming messages and process the buffer after welcome, so sending early subscription requests should work as expected.

@javan

This comment has been minimized.

Member

javan commented Apr 2, 2016

Looking great! Any issues deploying the server-side changes while clients still have the previous .js assets running?

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Apr 2, 2016

@javan There shouldn't be. If the client doesn't send any protocols it wants to use, the server won't either.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -53,6 +59,8 @@ class ActionCable.Connection
@isState("open", "connecting")
# Private

This comment has been minimized.

@javan

javan Apr 2, 2016

Member

Sorry to nitpick, but add a newline after this comment please. Indicates that all methods below are "private", not just the isPorotocolSupported method.

This comment has been minimized.

@danielrhodes

danielrhodes Apr 2, 2016

Contributor

👍

@danielrhodes danielrhodes force-pushed the danielrhodes:actioncable-websocket-protocols branch Apr 3, 2016

@danielrhodes

This comment has been minimized.

Contributor

danielrhodes commented Apr 3, 2016

Squashed the commits for squashlover7, err I mean @maclover7 :-P

P.S. I've actually started to like squashing now too, even if it's a conspiracy to keep everybody's commit numbers low to suppress rankings.

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
@@ -74,10 +83,12 @@ class ActionCable.Connection
events:
message: (event) ->
return if not @isActive

This comment has been minimized.

@javan

javan Apr 4, 2016

Member

This isn't right. @isActive is a function, not a property.

I think we want to check that the WebSocket is using a supported protocol here so messages aren't received during the brief window after successfully connecting using an unsupported protocol.

message: (event) ->
  return unless @isSupportedProtocol()
  ...

This comment has been minimized.

@danielrhodes

danielrhodes Apr 5, 2016

Contributor

👍

@javan

View changes

actioncable/app/assets/javascripts/action_cable/connection.coffee Outdated
ActionCable.log("WebSocket got '#{@getProtocol()}' as the chosen protocol. Stopping monitor and disconnecting.")
@close(allowReconnect: false)
return
ActionCable.log("WebSocket using '#{@getProtocol()}' subprotocol.")

This comment has been minimized.

@javan

javan Apr 4, 2016

Member

Not loving how this method reads now. The early return only guards against logging. I suggest:

open: ->
  ActionCable.log("WebSocket onopen event, using '#{@getProtocol()}' subprotocol")
  @disconnected = false
  unless @isProtocolSupported()
    ActionCable.log("Protocol is unsupported. Stopping monitor and disconnecting.")
    @close(allowReconnect: false)

This comment has been minimized.

@danielrhodes

danielrhodes Apr 5, 2016

Contributor

👍

Added protocol negotiation
This is primarily for backwards compatibility for when
or if the protocol is changed in future versions.

If the server fails to respond with an acceptable
protocol, the client disconnects and disables
the monitor.

@danielrhodes danielrhodes force-pushed the danielrhodes:actioncable-websocket-protocols branch to cbd15da Apr 5, 2016

@javan

This comment has been minimized.

Member

javan commented Apr 5, 2016

OK to merge as far as I'm concerned. Thanks for sticking with this one @danielrhodes!

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 5, 2016

Fantastic work @danielrhodes!

@jeremy jeremy merged commit cbd15da into rails:master Apr 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jeremy added a commit that referenced this pull request Apr 5, 2016

Merge pull request #24224 from danielrhodes/actioncable-websocket-pro…
…tocols

ActionCable protocol negotiation
@kaspth

This comment has been minimized.

Member

kaspth commented Apr 5, 2016

Well done! ❤️

@cdurante

This comment has been minimized.

cdurante commented Apr 6, 2016

is this typo?

I see the the function isProtocolSupported, but not isSupportedProtocol

[Error] TypeError: this.isSupportedProtocol is not a function. (In 'this.isSupportedProtocol()', 'this.isSupportedProtocol' is undefined) message (connection.self-7605b1bcff260663a68e0847ec3cfece34a28e2cb3c61fb59bcac2dfab6b6ad7.js:131) (anonymous function)

@jeremy

This comment has been minimized.

Member

jeremy commented Apr 6, 2016

Sure is. Thanks @cdurante!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment