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

Better protect against concurrent error handling for async requests #32340

Closed
rstoyanchev opened this issue Feb 28, 2024 · 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

rstoyanchev commented Feb 28, 2024

Section 3.2.2.4 of the Servlet spec indicates the request and response objects are not guaranteed to be thread safe, and the application must ensure that access to the request and response objects are thread safe. This is reasonably straight forward for applications to handle, if they even write concurrently, which may not be the case. However, the Servlet container may also trigger an AsyncListener#onError notification at any time as a result of network issues, which Spring MVC delegates to CallableProcessingInterceptors and DeferredProcessingInterceptors before performing an ASYNC dispatch for final handling of the error.

The onError comes on a Servlet container thread and may compete with the application write that would also fail. Currently, WebAsyncManager is not sufficiently well protected. For example, the failed write may be first to set the concurrentResult, and soon after perform an ASYNC dispatch. Meanwhile onError may return quickly, noticing the result is set and not taking any action in which case the Servlet container performs an ERROR dispatch that would race with the ASYNC dispatch. Another possibility is that onError is first to set the concurrentResult, which is later cleared during handling in the resulting dispatch, and that could trigger an additional ASYNC dispatch if the write fails after that.

The recently reported #32042 is related to such a race. The fix prevented an ASYNC dispatch if the error implies a disconnected client. However, that depends on the exception being recognized as such (also not wrapped by the application), and also precludes any ASYNC dispatch for final error handling where at least one should go through. We need a more conclusive fix for this.

In addition to that, the spec requires the request and response objects to only be accessed within the object’s life cycle. So as part of this work, we should also ensure compliance with this rule by wrapping the ServletOutputStream to reject further use, rather than expect all applications to synchronize with onError notifications in order to be compliant with the rule.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels Feb 28, 2024
@rstoyanchev rstoyanchev added this to the 6.1.5 milestone Feb 28, 2024
@rstoyanchev rstoyanchev self-assigned this Feb 28, 2024
@rstoyanchev rstoyanchev added for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels Feb 28, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x for: backport-to-5.3.x Marks an issue as a candidate for backport to 5.3.x labels Feb 28, 2024
rstoyanchev added a commit that referenced this issue Mar 3, 2024
The wrapped response prevents use after AsyncListener onError or completion
to ensure compliance with Servlet Spec 2.3.3.4.

The wrapped response is applied in RequestMappingHandlerAdapter.

The wrapped response raises AsyncRequestNotUsableException that is now
handled in DefaultHandlerExceptionResolver.

See gh-32340
rstoyanchev added a commit that referenced this issue Mar 3, 2024
1. Use state transitions
2. Increase synchronized scope in setConcurrentResultAndDispatch

See gh-32340
rstoyanchev added a commit that referenced this issue Mar 4, 2024
In addition to using the ServletOutputStream, it's also possible to call
ServletResponse#flushBuffer, so the ServletOutputStream wrapper logic needs
to apply there as well.

See gh-32340
rstoyanchev added a commit that referenced this issue Mar 7, 2024
webmvc.fn now also uses the StandardServletAsyncWebRequest wrapped response
to enforce lifecycle rules from Servlet spec (section 2.3.3.4).

See gh-32340
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

1 participant