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

Fix wrong connection close #3830

Merged

Conversation

tomas-langer
Copy link
Member

@tomas-langer tomas-langer commented Jan 21, 2022

Resolves #3829

Entity is consumed for any request that completed and did not subscribe (including JAX-RS).
We were consuming the entity anyway at a later stage (when we already did a connection close). For connection:close cases, entity is ignored if not consumed.

This keeps original handling and improved it even for requests with more than one chunk in request entity, while simplifying the code.

Signed-off-by: Tomas Langer tomas.langer@oracle.com

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
@tomas-langer tomas-langer self-assigned this Jan 22, 2022
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
danielkec
danielkec previously approved these changes Jan 25, 2022
@barchetta barchetta added this to the 2.4.2 milestone Jan 26, 2022
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
Signed-off-by: Tomas Langer <tomas.langer@oracle.com>
requestEntityAnalyzed = requestEntityAnalyzed.thenApply(listener -> {
LOGGER.fine(() -> log("Writing headers %s", status));
Copy link
Member

@spericas spericas Feb 2, 2022

Choose a reason for hiding this comment

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

Given the assignment in the line above, shouldn't we rename
requestEntityAnalyzed? Its name is too close to the new one added, yet its completion reflects more than just the request entity being processed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure I understand - it is the same future. We just need to keep the original instance and complete on it, as the "last" isntance in the field will not reflect thenApply and other methods registered on previous instances.

Copy link
Member

Choose a reason for hiding this comment

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

That's exactly the point, the last instance on this field is related to the writing of the response and no longer the analysis of the request entity (as its name suggests). My comment is just about naming, not semantics. Certainly not a stopper.

@tomas-langer tomas-langer merged commit 2a1dd6b into helidon-io:helidon-2.x Feb 3, 2022
@tomas-langer tomas-langer deleted the 3829-connection-close branch February 3, 2022 17:41
tomas-langer added a commit to tomas-langer/helidon that referenced this pull request Feb 3, 2022
* Fix wrong connection close
* Reduced logging overhead. Small improvements of tests.
* Consume request entity on valid responses.

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>

(cherry picked from commit 2a1dd6b)
tomas-langer added a commit that referenced this pull request Feb 7, 2022
* Fix wrong connection close
* Reduced logging overhead. Small improvements of tests.
* Consume request entity on valid responses.

Signed-off-by: Tomas Langer <tomas.langer@oracle.com>

(cherry picked from commit 2a1dd6b)
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.

4 participants