Merge pull request #24259 from jeremy/cable/disconnected-client-errors

Cable: Gracefully handle disconnected clients

5 files changed, 136 insertions, 1 deletions

diff --git a/actioncable/lib/action_cable/connection/client_socket.rb b/actioncable/lib/action_cable/connection/client_socket.rb
index 9e4dbcd6e6..7d6de78582 100644
--- a/actioncable/lib/action_cable/connection/client_socket.rb
+++ b/actioncable/lib/action_cable/connection/client_socket.rb
@@ -71,6 +71,8 @@ module ActionCable
 
       def write(data)
         @stream.write(data)
+      rescue => e
+        emit_error e.message
       end
 
       def transmit(message)
diff --git a/actioncable/lib/action_cable/connection/faye_client_socket.rb b/actioncable/lib/action_cable/connection/faye_client_socket.rb
index c9139b6858..47d09a9e14 100644
--- a/actioncable/lib/action_cable/connection/faye_client_socket.rb
+++ b/actioncable/lib/action_cable/connection/faye_client_socket.rb
@@ -36,6 +36,7 @@ module ActionCable
           @faye.on(:open)    { |event| @event_target.on_open }
           @faye.on(:message) { |event| @event_target.on_message(event.data) }
           @faye.on(:close)   { |event| @event_target.on_close(event.reason, event.code) }
+          @faye.on(:error)   { |event| @event_target.on_error(event.message) }
         end
     end
   end
diff --git a/actioncable/lib/action_cable/connection/stream.rb b/actioncable/lib/action_cable/connection/stream.rb
index 2d97b28c09..0cf59091bc 100644
--- a/actioncable/lib/action_cable/connection/stream.rb
+++ b/actioncable/lib/action_cable/connection/stream.rb
@@ -29,7 +29,7 @@ module ActionCable
       def write(data)
         return @rack_hijack_io.write(data) if @rack_hijack_io
         return @stream_send.call(data) if @stream_send
-      rescue EOFError
+      rescue EOFError, Errno::ECONNRESET
         @socket_object.client_gone
       end
 
diff --git a/actioncable/test/connection/client_socket_test.rb b/actioncable/test/connection/client_socket_test.rb
new file mode 100644
index 0000000000..dd730e348f
--- /dev/null
+++ b/actioncable/test/connection/client_socket_test.rb
@@ -0,0 +1,65 @@
+require 'test_helper'
+require 'stubs/test_server'
+
+class ActionCable::Connection::StreamTest < ActionCable::TestCase
+  class Connection < ActionCable::Connection::Base
+    attr_reader :websocket, :subscriptions, :message_buffer, :connected
+    attr_reader :errors
+
+    def initialize(*)
+      super
+      @errors = []
+    end
+
+    def connect
+      @connected = true
+    end
+
+    def disconnect
+      @connected = false
+    end
+
+    def send_async(method, *args)
+      send method, *args
+    end
+
+    def on_error(message)
+      @errors << message
+    end
+  end
+
+  setup do
+    @server = TestServer.new
+    @server.config.allowed_request_origins = %w( http://rubyonrails.com )
+  end
+
+  test 'delegate socket errors to on_error handler' do
+    skip if ENV['FAYE'].present?
+
+    run_in_eventmachine do
+      connection = open_connection
+
+      # Internal hax = :(
+      client = connection.websocket.send(:websocket)
+      client.instance_variable_get('@stream').expects(:write).raises('foo')
+      client.expects(:client_gone).never
+
+      client.write('boo')
+      assert_equal %w[ foo ], connection.errors
+    end
+  end
+
+  private
+    def open_connection
+      env = Rack::MockRequest.env_for '/test',
+        'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket',
+        'HTTP_HOST' => 'localhost', 'HTTP_ORIGIN' => 'http://rubyonrails.com'
+      env['rack.hijack'] = -> { env['rack.hijack_io'] = StringIO.new }
+
+      Connection.new(@server, env).tap do |connection|
+        connection.process
+        connection.send :handle_open
+        assert connection.connected
+      end
+    end
+end
diff --git a/actioncable/test/connection/stream_test.rb b/actioncable/test/connection/stream_test.rb
new file mode 100644
index 0000000000..d5aad63648
--- /dev/null
+++ b/actioncable/test/connection/stream_test.rb
@@ -0,0 +1,67 @@
+require 'test_helper'
+require 'stubs/test_server'
+
+class ActionCable::Connection::StreamTest < ActionCable::TestCase
+  class Connection < ActionCable::Connection::Base
+    attr_reader :websocket, :subscriptions, :message_buffer, :connected
+    attr_reader :errors
+
+    def initialize(*)
+      super
+      @errors = []
+    end
+
+    def connect
+      @connected = true
+    end
+
+    def disconnect
+      @connected = false
+    end
+
+    def send_async(method, *args)
+      send method, *args
+    end
+
+    def on_error(message)
+      @errors << message
+    end
+  end
+
+  setup do
+    @server = TestServer.new
+    @server.config.allowed_request_origins = %w( http://rubyonrails.com )
+  end
+
+  [ EOFError, Errno::ECONNRESET ].each do |closed_exception|
+    test "closes socket on #{closed_exception}" do
+      skip if ENV['FAYE'].present?
+
+      run_in_eventmachine do
+        connection = open_connection
+
+        # Internal hax = :(
+        client = connection.websocket.send(:websocket)
+        client.instance_variable_get('@stream').instance_variable_get('@rack_hijack_io').expects(:write).raises(closed_exception, 'foo')
+        client.expects(:client_gone)
+
+        client.write('boo')
+        assert_equal [], connection.errors
+      end
+    end
+  end
+
+  private
+    def open_connection
+      env = Rack::MockRequest.env_for '/test',
+        'HTTP_CONNECTION' => 'upgrade', 'HTTP_UPGRADE' => 'websocket',
+        'HTTP_HOST' => 'localhost', 'HTTP_ORIGIN' => 'http://rubyonrails.com'
+      env['rack.hijack'] = -> { env['rack.hijack_io'] = StringIO.new }
+
+      Connection.new(@server, env).tap do |connection|
+        connection.process
+        connection.send :handle_open
+        assert connection.connected
+      end
+    end
+end