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

Ensure AsyncListener#onError does not return until dispatch completes #34192

Closed
rstoyanchev opened this issue Jan 3, 2025 · 0 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@rstoyanchev
Copy link
Contributor

When a controller returns Flux<ServerSentEvent>, SseEmitter is used to write events. If the connection drops, the write fails with IOException in a Spring MVC TaskExecutor thread while in parallel we also get a Servlet container AsyncListener#onError callback.

In #32340, WebAsyncManager was enhanced to be protected in this race, so only one thread can dispatch. However, it is also crucial to ensure the onError callback holds up until the dispatch completes regardless of who wins. WebAsyncManager#setConcurrentResultAndDispatch guarantees this through synchronization, but when using DeferredResult, the onError callback may not call that method at all if DeferredResult error handling finds the result is already set by the competing thread. As a result, the onError callback may exit the container thread before dispatch in the Spring MVC thread compltes, which leads to the stacktrace in #33421 (comment) and also in #34188.

The issue does not apply when using Callable, e.g. via StreamingResponseBody as in that case setConcurrentResultAndDispatch is always called.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels Jan 3, 2025
@rstoyanchev rstoyanchev added this to the 6.1.17 milestone Jan 3, 2025
@rstoyanchev rstoyanchev self-assigned this Jan 3, 2025
@snicoll snicoll modified the milestones: 6.1.17, 6.2.2 Jan 6, 2025
@rstoyanchev rstoyanchev added the for: backport-to-6.1.x Marks an issue as a candidate for backport to 6.1.x label Jan 6, 2025
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.1.x Marks an issue as a candidate for backport to 6.1.x labels Jan 6, 2025
rstoyanchev added a commit that referenced this issue Jan 6, 2025
rstoyanchev added a commit that referenced this issue Jan 6, 2025
There is no need to set the DeferredResult from WebAsyncManager in an
onError notification because it is already done from the Lifecycle
interceptor in DeferredResult.

See gh-34192
rstoyanchev added a commit that referenced this issue Jan 6, 2025
rstoyanchev added a commit that referenced this issue Jan 6, 2025
There is no need to set the DeferredResult from WebAsyncManager in an
onError notification because it is already done from the Lifecycle
interceptor in DeferredResult.

See gh-34192
rstoyanchev added a commit that referenced this issue Jan 6, 2025
On connection loss, in a race between application thread and onError
callback trying to set the DeferredResult and dispatch, the onError
callback must not exit until dispatch completes. Currently, it may do
so because the DeferredResult has checks to bypasses locking or even
trying to dispatch if result is already set.

Closes gh-34192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

2 participants