Skip to content

Conversation

@geoand
Copy link
Contributor

@geoand geoand commented Nov 12, 2025

P.S. This is a breaking change because there might be code out there that depends on the side effects of various handlers always running (like code in response filters)

@quarkus-bot

This comment has been minimized.

@geoand geoand requested review from FroMage and Copilot November 13, 2025 05:44
Copilot finished reviewing on behalf of geoand November 13, 2025 05:47

This comment was marked as spam.

@quarkus-bot
Copy link

quarkus-bot bot commented Nov 13, 2025

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 61d1b10.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@FroMage
Copy link
Member

FroMage commented Nov 13, 2025

Meh. I don't think there's any expectation that response handlers run when the connection is closed. In fact I doubt this is even defined or specified.

There could be people asking for callbacks to be run after the connection closes, for cleanup. Like, making sure the TX is rolled back (but this happens in interceptors, so that should not be interrupted and complete, even with this change, which only skips the remaining steps, it does not interrupt any running one), or closing connections to the DB perhaps?

I'm not entirely sure what sort of handlers we have after the endpoint, and whether we can afford to skip them on connection loss. Do you have any sample?

Comment on lines +1355 to +1356
try {
context.discardRemaining();
Copy link
Member

Choose a reason for hiding this comment

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

Heh, I was going to say exactly the same thing. I'm impressed.

I'm not sure what thread invokes the close handler (I guess/hope the same request's context/thread) and what other thread might be running the request (especially in the case of blocking requests).

So this might indeed have more than one thread interact with the handler position in parallel. Needs to be checked.

@geoand
Copy link
Contributor Author

geoand commented Nov 13, 2025

I don't think there's any expectation that response handlers run when the connection is closed. In fact I doubt this is even defined or specified.

Agreed

Like, making sure the TX is rolled back (but this happens in interceptors, so that should not be interrupted and complete, even with this change, which only skips the remaining steps, it does not interrupt any running one), or closing connections to the DB perhaps?

Sure, but those are not REST handlers and would likely be handled in a different way

I'm not entirely sure what sort of handlers we have after the endpoint, and whether we can afford to skip them on connection loss. Do you have any sample?

Most prominent are ResponseHandler, ResponseWriterHandler ResourceResponseFilterHandler

@geoand geoand merged commit c99ac16 into quarkusio:main Nov 13, 2025
126 of 128 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.31 - main milestone Nov 13, 2025
@geoand geoand deleted the quarkus-rest-close-handlers branch November 13, 2025 11:42
@FroMage
Copy link
Member

FroMage commented Nov 13, 2025

Yeah, those should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants