on_close proof of concept

[chewi]

I felt a little bad that you'd gone and done all that work as I wasn't entirely sure whether I needed this yet. I was merely asking to see whether it was even worth considering. It would be a useful feature to have in any case but I really hate polling so I spent a little more time to see whether I could improve on that. I knew from prior experience that the server generally knows when the client has explicitly disconnected but I needed to determine exactly where it was waiting for that to happen. I tracked it down to the connection.each_request line. I then hacked up a way to pass through a call back from the application. It would look something like this.

eventsource "/sse" do
  # Send some stuff and leave connection open.
  responder.on_close = proc do
    logger.fatal "Client has gone away!"
  end
end

You may want to improve on the syntax a bit but this at least shows that it works.

[chewi]

On further testing, strange things seem to happen if you try to close the socket manually. Not sure why. Maybe you'd have a better idea about that.

[chewi]

I wonder if this is actually a bug. If you call es.close then Reel seems to hang here until Angelo exits.

[kenichi]

@chewi i think this is a good insight. you've found the right place in the code where a "client closed eventsource socket" would fall down to. i've been thinking of refactoring responder API a bit anyways, and i also think a mix of our two attempts/proofs at this will yield a good result. i don't think you would need to close the socket in the on_close handler, because it should already be closed. however, the stash accessors run a .reject! &:closed? without the change from the checking_sses branch, so i think a mix of the two branches will give us something like this:

eventsource '/' do |es|
  responder.on_close = ->{ sses(false).remove_socket es }
  sses << es
end

i'll put up another PR with the merge of these two changes, plus my thoughts on responder, and ping you then. thanks again for all your interest and help!

[chewi]

Sounds good. I didn't think about cleaning up the socket afterwards but I guess we might as well take advantage of this hook.

I didn't mean calling es.close inside the on_close handler. I tried calling it after defining the handler to see whether it triggered when the connection is closed by the server.

eventsource "/sse" do
  responder.on_close = proc do
    logger.fatal "Client has gone away!"
  end
  es.close
end

It does but only when Angelo exits! For that to even work, I had to put the on_close.call stuff in an ensure block because of a Celluloid::Task::TerminatedError exception that is raised when Angelo does exit. That's not important though, the point is that a thread is getting stuck and I think this was already happening before I started making changes. Closing the socket before the eventsource block has a chance to call halt seemed like a possible cause but I tried to delay the close and it still gets stuck. I'd like to look into this more but haven't had a chance.

[kenichi]

interesting! ok, i misunderstood about when you were trying to close. i think the server closing an SSE socket at all is kinda weird, since the client would be trying to reconnect automatically. would it be better to send the client a "please, close this?" event? i don't think i ever call socket.close specifically from angelo, unless it's in a rescue and not already closed. i've got a branch going, and am looking at ways to make the on_close block more static (i.e. not created on every request)

[kenichi]

@chewi https://github.com/kenichi/angelo/compare/sse_on_close

[chewi]

Weird, yet you include it in your examples. (-; I had thought about adding a "close" event but currently I have this so it won't reconnect. I'll reconsider whether this is appropriate.

esource.onerror = function(event) {
  esource.close();
};

Making it more static may help performance but I would like the option of being able to define it each time because that way you get the binding from the request, which is very useful.

[kenichi]

i came around to that same conclusion after thinking about this more last night. :)

[kenichi]

thanks again!