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

Add ErrorHandlerFunc #183

Merged
merged 4 commits into from
Jan 15, 2016
Merged

Add ErrorHandlerFunc #183

merged 4 commits into from
Jan 15, 2016

Conversation

jcorbin
Copy link
Contributor

@jcorbin jcorbin commented Jan 14, 2016

A simple corollary of HandlerFunc which allows the wrapped function to return
an error and handles returning a minimal error response.

r @prashantv @akshayjshah

@jcorbin
Copy link
Contributor Author

jcorbin commented Jan 14, 2016

Towards #173

@prashantv
Copy link
Contributor

Code looks good, can we add a test that uses the ErrorHandlerFunc. TestServerBusy in connection_test is an example that tests errors being returned.

@jcorbin
Copy link
Contributor Author

jcorbin commented Jan 14, 2016

Updated TestServerBusy to use; here's an idea for how we could start pivoting the existing raw.Wrap utility:

diff --git a/raw/handler.go b/raw/handler.go
index 5e92032..9ff7493 100644
--- a/raw/handler.go
+++ b/raw/handler.go
@@ -87,22 +87,15 @@ func WriteResponse(response *tchannel.InboundCallResponse, resp *Res) error {

 // Wrap wraps a Handler as a tchannel.Handler that can be passed to tchannel.Register.
 func Wrap(handler Handler) tchannel.Handler {
-       return tchannel.HandlerFunc(func(ctx context.Context, call *tchannel.InboundCall) {
+       return tchannel.ErrorHandlerFunc(func(ctx context.Context, call *tchannel.InboundCall) error {
                args, err := ReadArgs(call)
                if err != nil {
-                       handler.OnError(ctx, err)
-                       return
+                       return err
                }
-
                resp, err := handler.Handle(ctx, args)
-               response := call.Response()
                if err != nil {
-                       resp = &Res{
-                               SystemErr: err,
-                       }
-               }
-               if err := WriteResponse(response, resp); err != nil {
-                       handler.OnError(ctx, err)
+                       return err
                }
+               return WriteResponse(response, resp)
        })
 }

My thoughts are thus:

  • raw.Wrap and co seem really heavy handed to me
  • they don't recompose or reuse well (e.g. to my streaming case)
  • allowing a handler to implement custom error handling for InboundCall arg reading or InboundCallResponse arg writing seems like a pile of YAGNI to me (if not at least a source of non-standard behavior)
  • so that means the raw.Wrap handler interface could simplify down to a single function; could well just be this new ErrorHandlerFunc plus use of the upcoming WithArg23 utility (or even raw.ReadArgs if we prefer)

Ideally we'd share as much of this sort of code between all as-es (raw, http, thrift, json, etc).

@jcorbin jcorbin force-pushed the error_handler_func branch from 880baca to 571f150 Compare January 14, 2016 20:01
@prashantv
Copy link
Contributor

I'm happy with getting rid of raw.Wrap as well.

However, ErrorHandlerFunc does force how errors are handled (e.g. logging currently) which I don't think is always appropriate.

E.g. this test actually means that a handler returning ErrServerBusy would log an error, which is probably not what you want (especially if you're busy and the error path uses more resources).

The tests should have caught this extra log message, but log verification was broken recently by some interface changes. I'm fixing that in #186 which we should rebase this change off, which should cause the test to fail due to an unexpected log message.

// Handle calls f(ctx, call)
func (f ErrorHandlerFunc) Handle(ctx context.Context, call *InboundCall) {
if err := f(ctx, call); err != nil {
call.log.WithFields(ErrField(err)).Error("Handler error.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and below, we should add the handlerr name (which should be accessible via call) so the error log has more information

@jcorbin jcorbin force-pushed the error_handler_func branch from 571f150 to 9615a6d Compare January 14, 2016 22:26
@jcorbin
Copy link
Contributor Author

jcorbin commented Jan 14, 2016

Added error log annotate.

Good note about undue logging, I'll add some filtering after that eventual rebase so that we only log certain system errors; at the very least we want unexpected errors (and especially errors that are only coded as unexpected "by default" by not actually being SystemError s).

@prashantv
Copy link
Contributor

Merged other PR, so you should be able to rebase. I think only logging unexpected errors make sense.

@jcorbin jcorbin force-pushed the error_handler_func branch from 9615a6d to 397368e Compare January 15, 2016 21:41
@jcorbin
Copy link
Contributor Author

jcorbin commented Jan 15, 2016

Rebased and changed to only log unexpected errors (and to ignore any error returned by resp.SendSystemError as the thrift layer does).

@prashantv
Copy link
Contributor

code lgtm.

Would be good to also have a similar test with an unexpected error being returned just to have coverage on that branch. too

@jcorbin jcorbin force-pushed the error_handler_func branch from 397368e to 92f4099 Compare January 15, 2016 22:00
@jcorbin
Copy link
Contributor Author

jcorbin commented Jan 15, 2016

Added test case for unexpected handler error

@prashantv
Copy link
Contributor

test looks good, lgtm 🚢

jcorbin added a commit that referenced this pull request Jan 15, 2016
@jcorbin jcorbin merged commit 4662155 into master Jan 15, 2016
@jcorbin jcorbin deleted the error_handler_func branch January 15, 2016 22:12
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

Successfully merging this pull request may close these issues.

2 participants