Get panic cause as error in Error hooks #160

marioizquierdo · 2019-03-15T04:30:25Z

Currently, service method handlers make sure that panics still call Error hooks. However, the twirp.Error passed to the error hook has no information about the panic, it is simply an Internal error with Msg: "Internal service panic".

This PR adds a Cause to the error passed to the Error hooks, so it can be unwrapped with pkg/errors.Cause. The resulting error response is the same, with Msg: "Internal service panic", the information about the panic is only available in the server through accessing the cause.

// Given hooks with Error handling ...
hooks := &twirp.ServerHooks{
	Error: func(ctx context.Context, twerr twirp.Error) context.Context {		
		// Some errors are wrapped as Internal errors. Panics are also wrapped here.
		err := pkgerrors.Cause(twerr)

		// Before this PR
		twerr.Msg() //=> "Internal service panic"
		err.Error() //=> "twirp error internal: Internal service panic"

		// After this PR
		twerr.Msg() //=> "Internal service panic"
		err.Error() //=> shows the original error, or "panic: %v" if the panic value is not an error
	},
}

In addition, I made a little bit of refactor to simplify and explain the code that handles panics.

…n panics

marioizquierdo · 2019-03-15T04:31:55Z

_tools/src/golang.org/x/tools/go/ast/astutil/imports.go

-func AddImport(fset *token.FileSet, f *ast.File, ipath string) (added bool) {
- return AddNamedImport(fset, f, "", ipath)
+func AddImport(fset *token.FileSet, f *ast.File, path string) (added bool) {
+ return AddNamedImport(fset, f, "", path)


Sorry, all of this appeared when I first ran make test_core.
I wonder if I can git reset this, or if we should keep this diff around to keep tools up to date. I tried to mitigate some of this in another PR: #159 but that didn't update the tools. I'm not sure I understand what is going on here.

I think we need to rethink our build tooling in general, maybe moving to doing everything in Docker. I'll just accept these changes for now.

marioizquierdo · 2019-03-20T22:13:30Z

Fixes #163

…_handling

spenczar

Looks basically good - I agree with the goal here.

spenczar · 2019-03-25T17:30:42Z

internal/twirptest/service.twirp.go

+// The panic is not intercepted and should be handled in middleware to avoid empty responses.
+func handlePanics(ctx context.Context, resp http.ResponseWriter, hooks *twirp.ServerHooks) {
+ if r := recover(); r != nil {
+ err := errFromPanic(r) // allow to inspect error on Error hooks


These comments are overflowing way off to the right side. Could we get them to stay under ~100 characters, either by moving them above the lines they describe or by not vertically aligning them?

They seem to auto-align when generating code. I didn't realize that was happening. Fixing ...

spenczar · 2019-03-25T17:33:04Z

internal/twirptest/service_test.go

@@ -802,7 +803,7 @@ func TestPanickyApplication(t *testing.T) {
 h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 defer func() {
 if r := recover(); r == nil {
- t.Error("http server never panicked")
+ t.Error("http server never panicked") // also make sure it did panic


I don't think this comment is useful.

spenczar · 2019-03-25T17:34:03Z

internal/twirptest/service_test.go

+ errHookCalled = true
+ // The error should have a .Cause containing the panic value for inspection
+ err := pkgerrors.Cause(twerr)
+ if err.Error() != "This Is Fine Meme" {


Rather than testing the error string, you should be able to test whether err == panicValue directly.

spenczar · 2019-03-25T17:35:19Z

internal/twirptest/service_test.go

+ server := httptest.NewServer(h)
+ defer server.Close()
+
+ client := NewHaberdasherJSONClient(server.URL, http.DefaultClient)


Would be good to test both JSON and Protobuf code paths.

spenczar · 2019-03-25T17:35:49Z

internal/twirptest/service.twirp.go

 _ = writeErr
 }

- callResponseSent(ctx, hooks)
+ callResponseSent(ctx, hooks) // ResponseSent hook


Remove this comment

spenczar · 2019-03-25T17:36:21Z

internal/twirptest/service.twirp.go

- // hook, or just silently ignore the error.
- //
- // Logging is unacceptable because we don't have a user-controlled
+ // Most likely the connection is broken.


I prefer the old comment that was here, could you revert this change?

spenczar · 2019-03-25T17:37:42Z

internal/twirptest/service.twirp.go

@@ -381,32 +365,23 @@ func writeError(ctx context.Context, resp http.ResponseWriter, err error, hooks

 statusCode := twirp.ServerHTTPStatusFromErrorCode(twerr.Code())
 ctx = ctxsetters.WithStatusCode(ctx, statusCode)
- ctx = callError(ctx, hooks, twerr)
+ ctx = callError(ctx, hooks, twerr) // Error hook


Remove this comment

Would it make sense to rename callError to callErrorHook? and callResponseSent to callResponseSentHook? etc. No big deal, I just found "callError" to be a little misleading.

spenczar · 2019-03-25T17:40:59Z

internal/twirptest/service.twirp.go

+func (e *wrappedError) Cause() error { return e.cause }
+func (e *wrappedError) Error() string { return e.prefix + ": " + e.cause.Error() }
+
+// handlePanics makes sure that panics during rpc methods respond with Twirp Interna


typo: Internal

spenczar · 2019-03-25T17:42:40Z

internal/twirptest/service.twirp.go

+
+// handlePanics makes sure that panics during rpc methods respond with Twirp Interna
+// error responses (status 500) and trigger hooks with the panic wrapped as an error.
+// The panic is not intercepted and should be handled in middleware to avoid empty responses.


I'd phrase this differently. The panic is definitely intercepted, we just re-raise it.

Empty responses are kind of a separate issue that we should be able to deal with by flushing correctly.

Yes. I'll phrase it differently to be more clear. I found the empty response issue while working on this, it was not as clear to me before.

For reference, the empty responses issue is tracked here #163

spenczar

One minor bug in a test, but LGTM other than that.

spenczar · 2019-04-02T16:21:50Z

internal/twirptest/service_test.go

+ if err == nil {
+ t.Fatalf("%s client: err was expected for panicking handler, found nil", name)
+ }
+ if !errHookCalled {


Need to reset this variable to false after the first loop iteration

(fixed it myself)

spenczar · 2019-04-02T16:24:45Z

_tools/src/golang.org/x/tools/go/ast/astutil/imports.go

-func AddImport(fset *token.FileSet, f *ast.File, ipath string) (added bool) {
- return AddNamedImport(fset, f, "", ipath)
+func AddImport(fset *token.FileSet, f *ast.File, path string) (added bool) {
+ return AddNamedImport(fset, f, "", path)


I think we need to rethink our build tooling in general, maybe moving to doing everything in Docker. I'll just accept these changes for now.

spenczar · 2019-04-02T17:28:20Z

internal/twirptest/service_test.go

+ if err == nil {
+ t.Fatalf("%s client: err was expected for panicking handler, found nil", name)
+ }
+ if !errHookCalled {


(fixed it myself)

marioizquierdo added 3 commits March 14, 2019 12:00

WriteError calls writeHeader after response body to ensure it works o…

b9aa3f2

…n panics

Update code generation to better handle panics

8c58cbc

Make sure that response is written in the right order

fe63d42

marioizquierdo commented Mar 15, 2019

View reviewed changes

marioizquierdo added 2 commits March 25, 2019 10:16

Merge branch 'master' of github.com:twitchtv/twirp into better_panics…

0fb57c3

…_handling

run make test (generate)

962d995

spenczar suggested changes Mar 25, 2019

View reviewed changes

Address PR feedback

7943f51

marioizquierdo requested a review from spenczar March 29, 2019 06:24

spenczar suggested changes Apr 2, 2019

View reviewed changes

Reset test loop variable

eb43b01

spenczar approved these changes Apr 2, 2019

View reviewed changes

spenczar merged commit ad6d075 into master Apr 2, 2019

marioizquierdo mentioned this pull request Apr 2, 2019

Panics are not accessible in Error hooks #163

Closed

marioizquierdo deleted the better_panics_handling branch July 28, 2019 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get panic cause as error in Error hooks #160

Get panic cause as error in Error hooks #160

marioizquierdo commented Mar 15, 2019

marioizquierdo Mar 15, 2019 •

edited

Loading

spenczar Apr 2, 2019

marioizquierdo commented Mar 20, 2019

spenczar left a comment

spenczar Mar 25, 2019

marioizquierdo Mar 29, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

marioizquierdo Mar 29, 2019

spenczar Mar 25, 2019

spenczar Mar 25, 2019

marioizquierdo Mar 29, 2019

marioizquierdo Mar 29, 2019

spenczar left a comment

spenczar Apr 2, 2019

spenczar Apr 2, 2019

spenczar Apr 2, 2019

spenczar Apr 2, 2019

Get panic cause as error in Error hooks #160

Get panic cause as error in Error hooks #160

Conversation

marioizquierdo commented Mar 15, 2019

marioizquierdo Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioizquierdo commented Mar 20, 2019

spenczar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spenczar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marioizquierdo Mar 15, 2019 •

edited

Loading