Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed streams should probably result in errors, not recovered panics. #852

Closed
scottmmjackson opened this issue Mar 27, 2017 · 2 comments
Closed

Comments

@scottmmjackson
Copy link

scottmmjackson commented Mar 27, 2017

Proof of Concept here: https://gist.github.com/scottmmjackson/c35f4609750d9c6907a464780e55097a

Adding r.Use(gin.Recovery() does not change the outcomes below.

Do a curl https://localhost:8080 -k (especially with nghttp2 support built in) and CTRL+C (or TERM, or KILL) before the server has a chance to read a response. The result should be something like:

http2: panic serving [::1]:54808: http2: stream closed
// stacktrace here

Similarly a curl http://localhost:8181 that's terminated too early:

http: panic serving [::1]:54806: write tcp [::1]:8181->[::1]:54806: write: broken pipe
// stacktrace

That Context.JSON panics when the stream is closed too early means that in larger apps it's much more difficult for the app to gracefully degrade. The recovery handler is thus handling app-created panics (e.g.: the dev messed up) as well as panics that will necessarily occur in real-world use cases (e.g.: the user stopped loading the request, or a proxy reset the connection early).

It also pollutes stderr with stack traces that show that, yes, in fact, a user closed the socket before we could write the response.

@easonlin404
Copy link
Contributor

@scottmmjackson Could you help to replace gopkg.in/gin-gonic with github.com/gin-gonic?
I tested and had no problem with github.com/gin-gonic.

@aronatkins
Copy link

This appears to have been resolved by #2150, as the render helpers no longer panic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants