Skip to content

Commit

Permalink
event_source should handle errors properly
Browse files Browse the repository at this point in the history
and error_handler should take HIJACKING into consideration,
that would make things more consistent and therefore easier to
handle for event_source. add two tests to make sure it would
never deadlock and all errors are handled properly.

also, if we're signaling the mutex, that always means we are not
going to connect again. therefore whether the socket is closed or
not doesn't really matter. all we care is that it's done, and
should let go.
  • Loading branch information
godfat committed Nov 26, 2014
1 parent b749637 commit dbf460d
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 3 deletions.
10 changes: 8 additions & 2 deletions lib/rest-core/event_source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def close

def wait
raise RC::Error.new("Not yet started for: #{self}") unless mutex
mutex.synchronize{ condv.wait(mutex) until closed? } unless closed?
mutex.synchronize{ condv.wait(mutex) unless closed? } unless closed?
self
end

Expand Down Expand Up @@ -110,6 +110,12 @@ def onmessage_for sock
def reconnect
o = {REQUEST_HEADERS => {'Accept' => 'text/event-stream'},
HIJACK => true}.merge(opts)
client.get(path, query, o){ |sock| onopen(sock) }
client.get(path, query, o) do |sock|
if sock.nil? || sock.kind_of?(Exception)
onerror(sock)
else
onopen(sock)
end
end
end
end
6 changes: 5 additions & 1 deletion lib/rest-core/middleware/error_handler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,11 @@ def call env
def process res, err
RC::Promise.set_backtrace(err)
if res[ASYNC]
res.merge(RESPONSE_BODY => err)
if res[HIJACK]
res.merge(RESPONSE_SOCKET => err)
else
res.merge(RESPONSE_BODY => err)
end
else
raise err
end
Expand Down
21 changes: 21 additions & 0 deletions test/test_event_source.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,25 @@
end.start.wait
m.should.empty?
end

def mock_warning
mock(any_instance_of(RC::Promise)).warn(is_a(String)) do |msg|
msg.should.include?(Errno::ECONNREFUSED.new.message)
end
end

would 'not deadlock without ErrorHandler' do
mock_warning
client = RC::Simple.new.event_source('http://localhost:1')
client.onerror{ |e| e.should.eq nil }
client.start.wait
end

would 'not deadlock with ErrorHandler' do
mock_warning
client = RC::Universal.new(:log_method => false).
event_source('http://localhost:1')
client.onerror{ |e| e.should.kind_of?(Errno::ECONNREFUSED) }
client.start.wait
end
end

0 comments on commit dbf460d

Please sign in to comment.