Opt in PanicHandler #646
Opt in PanicHandler #646
Conversation
|
I'm still not in favor of merging this. Have you ever had a panic caused by fasthttp crash your process? All panics I have seen came from the user code in the handler. It shouldn't be up to fasthttp to always catch this for you. There is nothing wrong with adding a defer recover at the top of your handler if you want to catch these panics. What do you think @kirillDanshin? |
|
@erikdubbelboer I truly understand the lack of motivation for merging this. Having the user take responsibility is the correct way. Our handlers are just like you recommended, i.e. defer recover at the top. However, we had panics inside a ReadCloser Close method (body stream). This method is invoked outside the scope of the handler so we can't recover there. We can recover inside the Close method but it feels cumbersome, repetitive and error prone. Moreover, these ReadClosers may be outside our control *(think some 3rd party package). This opt-in callback would ease our job controlling the teardown of the process. Implementing one global function once, instead of constantly checking all flows and states beforehand. |
|
I'm wondering if We already have |
| if s.PanicHandler != nil { | ||
| defer func() { | ||
| s.cleanAfterHijackConn(r, c, hjc) | ||
| if r := recover(); r != nil { |
There was a problem hiding this comment.
this r shadows io.Reader defined above. I think we should rename this variable
| wp.workersCount-- | ||
| wp.lock.Unlock() | ||
| } | ||
|
|
||
| func (wp *workerPool) workerFunc(ch *workerChan) { |
There was a problem hiding this comment.
I don't really like that this change will silently affect users with PanicHandler == nil as well.
if wp.PanicHandler != nil for == nil would at least execute redundant JZ, and the if wp.PanicHandler == nil below implies something like
JNZ FUNCRET;
JMP WORKER_DONE;
FUNCRET:and that's all while the user doesn't even use this feature.
while I understand what (*wp).workerDone() is easier to read in the context of this feature, I'm not sure that this feature is really required, or at least that this implementation is optimal for both users who want the PanicHandler and who don't.
Again, I use fasthttp for several years now and I still didn't find a case when fasthttp would panic.
I'd like to hear some more thoughts about this PR.
There was a problem hiding this comment.
I also think it's better if we just handle panics from Response.SetBodyStream and Response.SetBodyStreamWriter and use s.logger().Printf("... and return without writing anything more in the response as we don't know what's written already.
For people who want more control it's not that hard to wrap the reader and handle these panics themselves.
This way we don't expose any extra API again.
There was a problem hiding this comment.
I agree with the above.
Moreover, #687 is definitely a step in the right direction for us. So closing this one.
erikdubbelboer
added
pending/submitter-response
and removed
pending/investigation
labels
|
Closing as not sure it's the right way to go. |
After recover() band-aids were removed (commit). Users were left with no possibility to act on a panic in a goroutine started inside fasthttp code. They can recover inside their own code. However, that might turn to be repetitive and cumbersome as they have to it on every handler, callback, custom interface implementation (ReadCloser) etc...
The discussion on this issue suggests that panic handling is the responsibility of the user. This PR allow the user to provide an opt-in callback which is called on a non nil return value from
recover(). Users are encouraged to re-panic in their implementation to allow the process to crash completely and not leave fasthttp internals in some unexpected state.Main use cases are error reporting and proper cleanup of resources upon an unexpected process termination.
Note: Currently, not all places creating goroutines are handled. I can add them later on if the general idea is accepted by the maintainers.