Skip to content

Commit

Permalink
Dont hold error lock when calling into error presenters
Browse files Browse the repository at this point in the history
This can result in a deadlock if error handling code calls GetErrors.
  • Loading branch information
vektah committed Nov 25, 2020
1 parent 0e12bfb commit 4628ef8
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 2 deletions.
5 changes: 3 additions & 2 deletions graphql/context_response.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ func AddErrorf(ctx context.Context, format string, args ...interface{}) {
func AddError(ctx context.Context, err error) {
c := getResponseContext(ctx)

presentedError := c.errorPresenter(ctx, ErrorOnPath(ctx, err))

c.errorsMu.Lock()
defer c.errorsMu.Unlock()

c.errors = append(c.errors, c.errorPresenter(ctx, ErrorOnPath(ctx, err)))
c.errors = append(c.errors, presentedError)
}

func Recover(ctx context.Context, err interface{}) (userMessage error) {
Expand Down
13 changes: 13 additions & 0 deletions graphql/context_response_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,16 @@ func TestAddError(t *testing.T) {
})
}
}

func TestGetErrorFromPresenter(t *testing.T) {
ctx := WithResponseContext(context.Background(), func(ctx context.Context, err error) *gqlerror.Error {
errs := GetErrors(ctx)

// because we are still presenting the error it is not expected to be returned, but this should not deadlock.
require.Len(t, errs, 0)
return DefaultErrorPresenter(ctx, err)
}, nil)

ctx = WithFieldContext(ctx, &FieldContext{})
AddError(ctx, errors.New("foo1"))
}

0 comments on commit 4628ef8

Please sign in to comment.