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

ServerHttpResponse sets the response statusCode correctly #5884

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jrhee17
Copy link
Contributor

@jrhee17 jrhee17 commented Aug 26, 2024

Motivation:

It seems like Armeria's spring integration response ServerHttpResponse is not always set correctly.
In detail, it seems like we can confirm that getStatusCode returns UNKNOWN unless setStatusCode is called explicitly.

  1. When a successful response is returned, we refer to the response headers used. This is similar to the upstream implementation which falls back to the response which is actually returned.
    https://github.com/spring-projects/spring-framework/blob/d2ea5b444812b4c55e00fecb7b6451073677061d/spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpResponse.java#L72-L76

  2. When Armeria returns a response instead of Spring, the response is never committed. Now, the doOnCancel is listened to and the response is committed accordingly.

Modifications:

  • ArmeriaServerHttpResponse#getStatusCode now also checks the response headers when returning a value
  • doOnCancel now also tries to set the response code and commit the ArmeriaServerHttpResponse

Result:

@jrhee17 jrhee17 added the defect label Aug 26, 2024
@jrhee17 jrhee17 added this to the 1.31.0 milestone Aug 26, 2024
@jrhee17 jrhee17 marked this pull request as ready for review August 27, 2024 00:53
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

.doOnCancel(() -> {
// If Armeria has already returned a response (e.g. RequestTimeout)
// make a best effort to reflect this in the returned response
if (ctx.log().isAvailable(RESPONSE_HEADERS)) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: We can do:

final RequestLog requestLog = ctx.log().getIfAvailable(RESPONSE_HEADERS);
if (requestLog != null) {...}

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

👍👍

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 this pull request may close these issues.

3 participants