Skip to content

Conversation

@lorban
Copy link
Contributor

@lorban lorban commented Oct 28, 2025

Let the ServletChannel do the last write upon completion.

Fixes #13849

@lorban lorban requested review from gregw and sbordet October 28, 2025 08:15
@lorban lorban self-assigned this Oct 28, 2025
@lorban lorban added the Bug For general bugs on Jetty side label Oct 28, 2025
@lorban lorban moved this to 🏗 In progress in Jetty 12.1.4 - FROZEN Oct 28, 2025
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

The approach looks good to me, but can we write a test case?

@lorban lorban requested a review from sbordet October 28, 2025 15:23
@lorban
Copy link
Contributor Author

lorban commented Oct 28, 2025

@sbordet sure, I added a test for each environment.

Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

The tests only test H1, for which we know there was not the problem.

Please move the test to jetty-eeN-test-client-transports so that we have a test for all transports about this.

@lorban
Copy link
Contributor Author

lorban commented Oct 28, 2025

@sbordet as I wrote here, the problem does happen with H1 too, it just doesn't have any visible effect. But doing two last writes is wrong and these tests catch that problem.

I can write a H2-specific test too but would be more complex as it would involve flow control and would be somewhat redundant with these ones.

@sbordet
Copy link
Contributor

sbordet commented Oct 28, 2025

@lorban can you clarify? If it does not have visible effects, then we cannot write a test case that would fail without the fix, which renders the test moot.

I would really try hard to write a test for all the transports.

@lorban
Copy link
Contributor Author

lorban commented Oct 29, 2025

I meant it doesn't have any visible effect for the client, there obviously are multiple way on the server to figure out that more than one last write was attempted.

I'll add a transport test, just to be safe.

@sbordet sbordet moved this from 🏗 In progress to 👀 In review in Jetty 12.1.4 - FROZEN Oct 29, 2025
@lorban lorban requested a review from sbordet October 31, 2025 13:24
… will perform the last write upon completion

Signed-off-by: Ludovic Orban <[email protected]>
@lorban lorban force-pushed the fix/12.1.x/13849-ServletContextResponse-duplicate-last-write-alt branch from 1a37e7f to 9ceda23 Compare November 4, 2025 14:51
sbordet
sbordet previously approved these changes Nov 5, 2025
…849-ServletContextResponse-duplicate-last-write-alt
…849-ServletContextResponse-duplicate-last-write-alt
@lorban lorban merged commit 63ddea9 into jetty-12.1.x Nov 6, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Jetty 12.1.4 - FROZEN Nov 6, 2025
@lorban lorban deleted the fix/12.1.x/13849-ServletContextResponse-duplicate-last-write-alt branch November 6, 2025 09:39
@lorban lorban moved this to 🏗 In progress in Jetty 12.0.30 - FROZEN Nov 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug For general bugs on Jetty side

Projects

No open projects
Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Connection reset on HTTP/2 when using CompressionHandler with default WindowRateControl

4 participants