ngauthier / tubesock

Websocket interface on Rack Hijack w/ Rails support
MIT License
620 stars 43 forks source link

Call open_handlers outside thread #51

Closed ghost closed 8 years ago

ghost commented 8 years ago

open_handlers exceptions are silently swallowed when inside thread. setting abort_on_exception inside that thread is not a solution.

ngauthier commented 8 years ago

This does not pass CI. I would prefer the handlers within the thread. Can we add abort on exception right after the new thread as we do in keepalive?

ghost commented 8 years ago

i must admit it was a quick dirty hack. can we simply call open handlers inside a begin/rescue block?

        begin
          @open_handlers.each(&:call)
        rescue => e
          @error_handlers.each{|eh| eh.call(e)}
          close if @close_on_error
        end
ghost commented 8 years ago

inside thread of course

ghost commented 8 years ago

@ngauthier, looks reasonable? have to update test_close_on_open_handler to check exception is passed to error handler rather than raised.

ngauthier commented 8 years ago

No, I would not like to do a naked rescue. Can you try adding abort on exception within the thread like this:

  def listen
    keepalive
    Thread.new do
      Thread.current.abort_on_exception = true
      begin
        @open_handlers.each(&:call)
        each_frame do |data|
          @message_handlers.each do |h|
            begin
              h.call(data)
            rescue => e
              @error_handlers.each{|eh| eh.call(e,data)}
              close if @close_on_error
            end
          end
        end
      ensure
        close
      end
    end
  end

Then in your open handler, try a simple raise 'hello world' and see if it is caught.

ghost commented 8 years ago

well, this was the first thing i did yesterday but did not like it is halting the entire process. second though was to put outside thread and let the code upper tubesock handle exceptions. but perhaps the optimal solution is to let error handlers do their job like they does on frames?

ngauthier commented 8 years ago

Hm, I think having it halt the entire process is OK, because there really should not be uncaught exceptions. I think if you are experiencing exceptions during the open handlers and message handlers, it would either be:

  1. Exceptions from your code, which you are responsible to catch in your handlers
  2. Exceptions from the websocket (like the handler started, then the client closed, then you tried to send data, and websocket library raises a send on closed socket)

For (2) we could rescue a very specific set of exceptions that are experienced from the websocket layer.

Could you explain more what exception you are experiencing?

ghost commented 8 years ago

yes, that are exceptions generated by my code. sure i can handle them in my code but the idea was to have a single point where they are handled - onerror handlers.

ngauthier commented 8 years ago

Ah I see. Following the WebSocket JavaScript protocol, onerror is when there is a WebSocket level error (like client disconnects). I think this would be useful for Tubesock as well.

But, I believe your errors are yours to handle, and it would be inappropriate for Tubesock to replicate Ruby's rescue behavior.

ghost commented 8 years ago

you right, let's just abort on error

ngauthier commented 8 years ago

Cool. I am glad this works for you as well. I do think onerror would be nice for socket errors, though!

ghost commented 8 years ago

indeed, makes perfect sense