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

TomcatService doesn't work Spring Boot error handling #3447

Closed
0x1306e6d opened this issue Apr 10, 2021 · 7 comments · Fixed by #3732
Closed

TomcatService doesn't work Spring Boot error handling #3447

0x1306e6d opened this issue Apr 10, 2021 · 7 comments · Fixed by #3732
Labels
Milestone

Comments

@0x1306e6d
Copy link
Contributor

0x1306e6d commented Apr 10, 2021

Hi, Armeria team.

I migrated a synchronous Spring Boot application with embedded Tomcat to use Armeria server. As I can't migrate all synchronous code to asynchronous and reactive code, I used TomcatService to connect with Armeria and Tomcat. And I found that the TomcatService does not handle error handling describe in the Spring Boot's document:

By default, Spring Boot provides an /error mapping that handles all errors in a sensible way, and it is registered as a “global” error page in the servlet container. For machine clients, it produces a JSON response with details of the error, the HTTP status, and the exception message. For browser clients, there is a “whitelabel” error view that renders the same data in HTML format (to customize it, add a View that resolves to error).

Similarly, @ControllerAdvice does not work well:

You can also define a class annotated with @ControllerAdvice to customize the JSON document to return for a particular controller and/or exception type, as shown in the following example: ...

Any JSON response is not respond from the Armeria server. And I guess it's because the "global" error page in the servlet container does not called.

I made an example in my repository. The example have two applications, one is a Spring Boot application with embedded Tomcat. And the other one is similar application but integrated with the Armeria. Tests in both applications describe expected behavior and the Armeria application's test is failed because JSON response does not exist.

@kojilin
Copy link
Member

kojilin commented Apr 28, 2021

For your example. The /baz will work well in the test if adding SimpleControllerAdvice to the @SpringBootTest's classes attribute. It's already works if you do bootRun and using any client because the advice is scanned correctly.

About the /bar 🤔 . Not sure why it doesn't work, maybe it register outside of connector(“global” error page)!? Maybe armeria user should do it by self using ExceptionHandlers.

@trustin trustin added the defect label Apr 28, 2021
@trustin trustin added this to the 1.8.0 milestone Apr 28, 2021
@0x1306e6d
Copy link
Contributor Author

0x1306e6d commented May 3, 2021

Thanks @kojilin !!

For your example. The /baz will work well in the test if adding SimpleControllerAdvice to the @SpringBootTest's classes attribute. It's already works if you do bootRun and using any client because the advice is scanned correctly.

I confirmed /baz succeeded after adding SimpleControllerAdvice. Let me update my example. Now there is only one to fix(?).

About the /bar 🤔 . Not sure why it doesn't work, maybe it register outside of connector(“global” error page)!? Maybe armeria user should do it by self using ExceptionHandlers.

Me too 🤔 yet 😂

@heowc
Copy link
Contributor

heowc commented May 11, 2021

I am interested in this issue and have tried debugging it. 😎

As a result of doing this, the problematic parts are as follows. Each module in the example produces different results here.

https://github.com/apache/tomcat/blob/9747a3a6334369deb9b5bef1b17b1fe0ce774cdf/java/org/apache/catalina/core/StandardHostValve.java#L157-L165

https://github.com/apache/tomcat/blob/9747a3a6334369deb9b5bef1b17b1fe0ce774cdf/java/org/apache/coyote/Response.java#L207-L215

I think this is because we made a pure response from TomcatService provided by Armeria.

When we create a pure response, ActionHook is empty.

https://github.com/apache/tomcat/blob/9747a3a6334369deb9b5bef1b17b1fe0ce774cdf/java/org/apache/coyote/Response.java#L108

And ActionHook is set only when it is made through Processor.

https://github.com/apache/tomcat/blob/9747a3a6334369deb9b5bef1b17b1fe0ce774cdf/java/org/apache/coyote/AbstractProcessor.java#L83-L92

We should be able to call the intended ErrorController by executing throwable(...) in the code mentioned above. I know the cause, but I’m not sure exactly how to fix it. 🤣

@trustin
Copy link
Collaborator

trustin commented May 12, 2021

Thanks for looking into this issue. Sounds like it's more complicated than initially thought. 😅 Maybe we need to:

  • build some fake AbstractProcessor implementation and invoke it; or
  • copy the error handling logic somehow from AbstractProcessor?

@ikhoon ikhoon modified the milestones: 1.8.0, 1.9.0 May 14, 2021
@heowc
Copy link
Contributor

heowc commented May 14, 2021

How about doing it in a different way?

As mentioned above, we can register an excption handler using Armeria. (Is it necessary to handle the white label as well..? 🤔)
Alternatively, webflux can use WebExceptionHandler (DefaultErrorWebExceptionHandler)

@trustin
Copy link
Collaborator

trustin commented Jun 5, 2021

As mentioned above, we can register an excption handler using Armeria. (Is it necessary to handle the white label
as well..?)

Two questions:

  • Who registers the exception handler using Armeria? TomcatService or a user?
  • What is 'white label'?

@heowc
Copy link
Contributor

heowc commented Jun 6, 2021

Who registers the exception handler using Armeria? TomcatService or a user?

user? 🤔

What is 'white label'?

It is one of the features provided by spring boot.

@ikhoon ikhoon modified the milestones: 1.9.0, 1.10.0 Jun 22, 2021
trustin pushed a commit that referenced this issue Aug 18, 2021
Motivation:

No `ActionHook` is set because `TomcarService` makes a pure `Request`. Due to this, default error handling does not work when integrating with Spring Boot.

Modifications:

- Add `ArmeriaProcessor` per `tomcat` module
- Add `ArmeriaEndpoint` to `tomcat8`
- `ArmeriaProcessor` and `ArmeriaEndpoint` are created with `MethodHandle`
- Add test

Result:

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

Successfully merging a pull request may close this issue.

5 participants