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

Issue #12272 - Potential deadlock with Vaadin. #12506

Merged

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Nov 8, 2024

Added test case.

@sbordet sbordet requested a review from lorban November 8, 2024 18:10
@sbordet sbordet linked an issue Nov 8, 2024 that may be closed by this pull request
lorban
lorban previously approved these changes Nov 11, 2024
Fixed the case where a GOAWAY followed by a TCP FIN was causing a race between closing the `EndPoint` and running the failure `Runnable` task.

The TCP FIN after the GOAWAY causes the streams to be failed on the server;
in turn, failing the streams generates failure `Runnable` tasks that are submitted to the HTTP/2 execution strategy;
however, the streams were destroyed before the failure `Runnable` tasks actually ran, so the `EndPoint` was closed;
closing the `EndPoint` would close the `HTTP2Connection`, which in turn would stop the execution strategy;
this lead to the fact that the failure `Runnable` tasks were never run.

Now, the failure `Runnable` tasks are invoked via `ThreadPool.executeImmediately()` rather than being submitted to the execution strategy.
This ensures that they would be run and not queued, even in case of lack of threads, so that they could unblock blocked reads or writes, freeing up blocked threads.

Additionally, improved `HTTP2Stream.onFailure()` to destroy the stream only after the failure tasks have completed.

Smaller other fixes to improve the code.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet force-pushed the fix/jetty-12.0.x/12272/h2-shutdown-pending-request branch from 7b9b156 to 713a308 Compare November 11, 2024 19:07
@sbordet sbordet requested a review from lorban November 11, 2024 19:19
Copy link
Contributor

@lorban lorban left a comment

Choose a reason for hiding this comment

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

Just a couple of nits.

await().atMost(5, TimeUnit.SECONDS).until(() -> !session.getEndPoint().isOpen());

// Cleanup.
dataList.forEach(Stream.Data::release);
Copy link
Contributor

Choose a reason for hiding this comment

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

The cleanup could be done right after awaiting !dataList.isEmpty()

await().atMost(5, TimeUnit.SECONDS).until(() -> !session.getEndPoint().isOpen());

// Cleanup.
dataList.forEach(Stream.Data::release);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

@sbordet sbordet merged commit 5f121a7 into jetty-12.0.x Nov 12, 2024
10 checks passed
@sbordet sbordet deleted the fix/jetty-12.0.x/12272/h2-shutdown-pending-request branch November 12, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Potential deadlock with Vaadin
2 participants