Better Result error handling #6153

richdougherty · 2016-05-13T07:23:20Z

When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error
handling was done with custom error handling code for the server.

This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of the server generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see #6098, #6099).

This pull request changes the signature of HttpErrorHandler's onServerError method to take a status code parameter. Previously the method always assumed a status code of 500. Now, the method takes a
status code parameter so that it can support additional status codes such as 505, which is needed by this pull request.

Play's error handling code now calls the new method with the status code. The old method is deprecated and no longer called. User code will need to upgrade. This is documented in the Play 2.6 migration documentation.

richdougherty · 2016-05-13T10:07:04Z

Cc @bbarkley.

bbarkley · 2016-05-13T17:02:10Z

Thanks for tackling this! In our case the most important concern is being able to differentiate between an Exception we've already seen in the filter chain vs one that has been thrown elsewhere that we're not aware of. In the filter chain we can notice failed Futures and have additional context such as the total processing time before failure and additional metadata on the request that upstream filters may have had. I assume that the onServerError handler is called with the original RequestHeader which wouldn't have that context, so onServerError isn't where we can do the majority of our error handling - it would be more of a place of last resort if we aren't able to handle it in the filter chain. Given this I think it's important to be able to know when it's being invoked by Play due to internal problems vs when it's passing through an Exception from user code.

Based on the diff it looks like looking for ServerResultException would give us this "is it ours or Play's?" answer, but in the Assets code a generic RuntimeException is being used. Is it worth having a PlayInternalException that any internal code could subclass or throw that would indicate that onServerError is being called due to an issue in Play that app code isn't aware of?

richdougherty · 2016-05-16T09:38:16Z

Hi @bbarkley. Thanks for your feedback. I agree that it makes sense for Play to have its own exceptions instead of throwing vanilla RuntimeExceptions. It should be pretty easy to replace the RuntimeExceptions with a bit of grepping. I think Play throws a quite few IllegalArgumentExceptions, IllegalStateExceptions and other standard Java exceptions. Are they a problem for your error handling code too?

gmethvin · 2016-05-16T16:56:24Z

framework/src/play/src/main/java/play/http/DefaultHttpErrorHandler.java

+    }
+
+    /**
+     * This method is no longer invoked by Play; override the version with the


Is the idea to backport some/all of this to 2.5.x/2.4.x? We can't just break existing code there so I'm not sure this is going to be the right approach.

An alternative might be to add a statusCode field to UsefulException.

If we backport, I was thinking of leaving the onServerError method the same. So the backported versions would get 90% of the changes, but the interface would remain the same. I would hack the one place where we need a non-500 server error.

Sure. It's definitely the right API to have the status code in the method call. We can address the hack when we do the backport. And I'm also reconsidering whether we need onDevServerError and onProdServerError (see the discussion on #6116).

bbarkley · 2016-05-16T21:58:44Z

Regarding the exceptions - if Play is internally throwing IllegalStateExceptions and similar it wouldn't be possible to differentiate between application exceptions throwing those types and Play's internal ones. It seems overkill to require that all those be changed to extend from a common Play exception type.

What do you think about adding a new API to the error handler trait - onInternalError or similar to be called when a non-application exception occurs? This could by default delegate to onServerError for apps that do all of their handling using an error handler.

gmethvin · 2016-05-17T06:35:43Z

Thinking about the future API for HttpErrorHandler, it would be nice to have something like:

trait HttpErrorHandler {
  def onError(request: RequestHeader, error: HttpError): Future[Result]
}

Where the HttpError class contains all the relevant metadata (error message, optional exception, etc.). Then you could even have a flag for whether it was an internal Play exception or an exception in your code, and potentially add other pieces of information in the future.

We can keep some of the utility methods in DefaultHttpErrorHandler, but as a user you could also just implement onError and pattern match on the error.

richdougherty · 2016-05-19T00:09:05Z

OK, after the discussion on this issue and some more offline discussion with others I realise that there are still lots of questions about how to do error handling in Play. I think it was premature of me to change the HttpErrorHandler interface. What I've done is split off the more general error handling discussion into a separate issue (#6171) and simplified this PR so it's now just a fix to improve error handling in some specific cases. Let's continue our discussion about error handling in Play over there and hopefully we can merge and backport this simpler PR.

gmethvin · 2016-05-20T22:50:35Z

...work/src/play-akka-http-server/src/main/scala/play/core/server/akkahttp/AkkaHttpServer.scala

  }

  /** Error handling to use during execution of a handler (e.g. an action) */
  private def handleHandlerError(tryApp: Try[Application], rh: RequestHeader, t: Throwable): Future[Result] = {


Do we still need this method?

Fixes playframework#6099. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see playframework#6098, playframework#6099).

richdougherty · 2016-05-24T22:41:30Z

Thanks for the feedback. I've updated this PR now.

richdougherty · 2016-05-30T23:47:26Z

@gmethvin, if you're happy with the changes I've made would you mind merging this PR? Thanks!

gmethvin · 2016-05-31T10:12:25Z

@richdougherty Since there are some conflicts, would you mind submitting additional PRs for the backports? Feel free to merge them yourself if there are no major issues.

richdougherty · 2016-05-31T11:20:13Z

OK, I'll handle the backports. Thanks for merging. :)

Fixes #6099. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see #6098, #6099).

This reverts commit d763b3d.

Fixes playframework#6099. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see playframework#6098, playframework#6099).

This is a backport of playframework#6153. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see playframework#6098, playframework#6099).

This is a backport of #6153. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see #6098, #6099).

This is a backport of playframework#6153. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see playframework#6098, playframework#6099).

* Better Result error handling This is a backport of #6153. When the backend server (Netty, Akka HTTP) discovers an error in a result (e.g. invalid headers) the server needs to abort processing the result and send an error result to the user. Previously, this error handling was done with custom error handling code for the server. This pull request changes the server behavior to use the application's HttpErrorHandler to generate the error result instead of generating a custom error response. Using the existing error handler makes the error handling more standardized. It also means that the application will be notified when an error occurs during result generation, which will make it easier to track errors that occur in Play applications (see #6098, #6099). * Skip slow failing Akka tests * Update MIMA filters

gmethvin reviewed May 16, 2016
View reviewed changes

richdougherty force-pushed the result-error-handling branch from 6fffb40 to 29c6b9a Compare May 18, 2016 21:42

richdougherty mentioned this pull request May 19, 2016

Improve error handling (HttpErrorHandler) #6171

Open

gmethvin reviewed May 20, 2016
View reviewed changes

richdougherty force-pushed the result-error-handling branch from 29c6b9a to 87ec67a Compare May 24, 2016 22:38

gmethvin merged commit e772e65 into playframework:master May 31, 2016

mkurz added this to the 2.5.4 milestone Jun 2, 2016

gmethvin added a commit that referenced this pull request Jun 2, 2016

Revert "Better Result error handling (#6153)"

1ff7bf3

This reverts commit d763b3d.

richdougherty mentioned this pull request Jun 2, 2016

Better Result error handling #6210

Merged

richdougherty mentioned this pull request Jun 8, 2016

Better Result error handling (2.4.x) #6223

Merged

guizmaii mentioned this pull request Mar 18, 2018

Implement a JsonHttpErrorHandler #8299

Merged

8 tasks

Uh oh!

Better Result error handling #6153

Better Result error handling #6153

Uh oh!

Conversation

richdougherty commented May 13, 2016

Uh oh!

richdougherty commented May 13, 2016

Uh oh!

bbarkley commented May 13, 2016

Uh oh!

richdougherty commented May 16, 2016

Uh oh!

gmethvin May 16, 2016

Choose a reason for hiding this comment

Uh oh!

richdougherty May 16, 2016

Choose a reason for hiding this comment

Uh oh!

gmethvin May 17, 2016

Choose a reason for hiding this comment

Uh oh!

bbarkley commented May 16, 2016

Uh oh!

gmethvin commented May 17, 2016

Uh oh!

richdougherty commented May 19, 2016

Uh oh!

gmethvin May 20, 2016

Choose a reason for hiding this comment

Uh oh!

richdougherty commented May 24, 2016

Uh oh!

richdougherty commented May 30, 2016

Uh oh!

gmethvin commented May 31, 2016

Uh oh!

richdougherty commented May 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants