search

ostinelli / net-http2

NetHttp2 is an HTTP/2 client for Ruby.
MIT License
141 stars 31 forks source link

Thread.abort_on_exception should not be used here #40

Closed spuyet closed 4 years ago

spuyet commented 5 years ago

Hello @ostinelli ,

I'm relaunching the debate about the socket exception management. The readme says:

It is RECOMMENDED to set the :error callback: if none is defined, the underlying socket thread may raise an error in the main thread at unexpected execution times.

The current code is not doing that actually, but killing the running process because of the abort_on_exception = true here.

I'm convinced that a library cannot kill a process because of an internal exception without letting the developper choose the right thing to do, like handling or not the exception.

A good example is the one already discussed here: https://github.com/ostinelli/apnotic/issues/17.

Sidekiq is using threads as workers to process enqueued jobs and a scheduler thread to push jobs to those workers. When you set abort_on_exception to true on a thread, it will kill the whole Sidekiq process on the next exception. The :error callback seems to be enough to me as it has access to the exception and let us do what we want with it. IMO, the right fix is to remove the abort_on_exception.

WDYT ? can I submit a PR about this ? Then we'll be able to test and discuss about it.

ostinelli commented 5 years ago

The current code is not doing that actually, but killing the running process because of the abort_on_exception = true here.

To my understanding, what you state is incorrect.

All errors generated in the socket thread are rescued, and when they are the callback_or_raise method is called.

This method, as its name states, will call the developer's callback (if specified, which is what the README recommends) OR, if no callback is specified, raise an error, in which case you want that error to be propagated to the main thread (hence the need for abort_on_exception = true).

I'm convinced that a library cannot kill a process because of an internal exception without letting the developper choose the right thing to do, like handling or not the exception.

Isn't this what the callback is for, letting the developer choose what to do with an error? The (closed) issue you refer to (https://github.com/ostinelli/apnotic/issues/17) was precisely the discussion that led to the addition of the callback.

Am I missing the point you want to make?