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

Fix Asynchronous Dispatch Logic in AwsAsyncContext with Spring's DispatcherServlet #631

Merged
merged 18 commits into from
Jan 30, 2024

Conversation

2012160085
Copy link
Contributor

@2012160085 2012160085 commented Sep 10, 2023

*Issue #, if available: #630

*Description of changes:

Problem

While examining #630, I found an issue within the asynchronous dispatch logic of AwsAsyncContext when interacting with Spring's DispatcherServlet.

  • Premature Re-dispatch: Currently, AwsAsyncContext prematurly triggers re-dispatch.
    When handling asynchronous requests with DispatcherServlet, DispatcherServlet'sdoDispatch should be called to complete the initial request. Only after the initial request concludes, should the doDispatch be called again for re-dispatch. However, in the present implementation, this sequence is disrupted. During the processing of the initial request by the doDispatch method, the dispatch method of AwsAsyncContext is invoked, which in turn directly calls the doFilter method for immediate re-dispatch. This leads to premature execution of the re-dispatch before the post-processing logic of doDispatch(which is handling the initial request) is executed.

  • EntityManager Bind Timing Issue: While using Spring JPA, due to the aforementioned issue, an error arises when trying to bind the EntityManager to threadLocal during the preHandle phase of the re-dispatch. This is because the EntityManager is already bound from the initial request, and the post-processing logic meant to unbind it hasn't been executed yet. As a result, attempting to bind it again triggers the following error:

Value [org.springframework.orm.jpa.EntityManagerHolder@38a6ca39] already bound to key [org.springframework.orm.jpa.LocalContainerEntityManagerFactoryBean@39296cef].

Changes

To resolve this, I made modifications to the SpringBootLambdaContainerHandler::handleRequest and AwsAsyncContext::dispatch by referencing the way requests are handled in apache.coyote.AbstractProcessorLight::process.
The snippet for the proposed solution is as follows:

protected void handleRequest(HttpServletRequest containerRequest, AwsHttpServletResponse containerResponse, Context lambdaContext) throws Exception {
    // ... [other code]
    doFilter(containerRequest, containerResponse, reqServlet);
    //added addtional filter logic
    if(requiresAsyncReDispatch(containerRequest)) {
        doFilter(containerRequest, containerResponse, reqServlet);
    }
    // ... [other code]
}
public void dispatch() {
    // ... [other code]
    //added dispatchStarted flag
    if (!dispatchStarted.get()) {
        dispatchStarted.set(true);
    } else {
        dispatched.set(true);
    // removed the line that directly calls doFilter
        notifyListeners(NotificationType.START_ASYNC, null);
    }
    // ... [other code]
}

By ensuring that the post-processing of the initial request is complete and only then re-invoking the doFilter when necessary, we can bypass the issues mentioned above.

I have not yet added test cases, and there are two test cases marked with @disable since AwsAsyncContext no longer maps requests to the servlet. I think we can talk about this solution or potentially explore better approaches.

By submitting this pull request

  • I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • I confirm that I've made a best effort attempt to update all relevant documentation.

Ensure post-processing logic is executed before re-dispatching during asynchronous request handling

* SpringBootLambdaContainerHandler
- Added logic within the handleRequest method to reprocess the request in cases where an asynchronous request requires re-dispatching.

* AwsAsyncContext
- Added an isDispatchStarted method that returns whether the dispatch has started or not.
- Removed the part where doFilter is directly called within the dispatch function.
*AwsAsyncContextTest
-Disabled servlet mapping tests as it no longer make servlet requests.
@mbfreder
Copy link
Contributor

Thanks for your contribution. I will review this today and let you know.

@mbfreder
Copy link
Contributor

Looks good to me. Could you add tests, please?

@mbfreder mbfreder requested a review from deki September 14, 2023 07:04
@2012160085
Copy link
Contributor Author

I added test cases

-Added jpaapp.JpaApplication for H2 and Spring Data JPA testing.
-Excluded JPA auto-configuration from other test apps to prevent interference.
-Implemented one async and one sync test case in JpaAppTest
@deki
Copy link
Collaborator

deki commented Sep 18, 2023

How do we plan to deal with the @Disabled test cases? I've not worked much with WebFlux but we need to make sure that the things implemented in #239 are still working.

-get reqServlet before doFilter for redispatch
-Added re-dispatch logic inside SpringLambdaContainerHandler's handleRequest
-Added test cases for async request
@2012160085
Copy link
Contributor Author

2012160085 commented Sep 18, 2023

How do we plan to deal with the @Disabled test cases? I've not worked much with WebFlux but we need to make sure that the things implemented in #239 are still working.

The two @Disabled test cases are testing whether AwsAsyncContext's dispatch correctly maps to the right servlet.
Specifically, it's testing this line to see whether the right servlet is input when calling doFilter.

However, in my commit, this line has removed from the dispatch function, and the responsibility for mapping the servlet has now shifted from AwsAsyncContext to SpringBootLambdaContainerHandler. I believe the disabled cases are adequately covered in the test cases of aws-serverless-java-container-springboot and springboot3.

However, as you pointed out, I seem to have overlooked testing whether the request path is properly amended when dispatch is called in AwsAsyncContextTest. I've added that test.

@2012160085
Copy link
Contributor Author

I missed that both springboot3 and spring handle async requests the same way. After looking over it again, I've updated the SpringLambdaContainerHandler in aws-serverless-java-container-spring and added some tests.

@2012160085
Copy link
Contributor Author

@deki Any further feedback on my update?

Copy link
Collaborator

@deki deki left a comment

Choose a reason for hiding this comment

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

AwsAsyncContext no longer depends on the AwsLambdaServletContainerHandler could you please remove the unused variable as well?

@deki
Copy link
Collaborator

deki commented Oct 23, 2023

Hey @2012160085,
sorry for the delay, I saw your comment on #630 and had therefore de-prioritized this PR.

I read https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#asynchronous-processing, looked at https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/AsyncContextImpl.java and noticed we may required additional fixes. The async processing capability should work within aws-serverless-java-container-core but with the proposed change you made calling filters exclusive to -spring and -springboot3. Filters for apps running -jersey and other potential implementations would no longer be called or am I missing something?

@2012160085
Copy link
Contributor Author

Hey @2012160085, sorry for the delay, I saw your comment on #630 and had therefore de-prioritized this PR.

I read https://jakarta.ee/specifications/servlet/6.0/jakarta-servlet-spec-6.0#asynchronous-processing, looked at https://github.com/apache/tomcat/blob/main/java/org/apache/catalina/core/AsyncContextImpl.java and noticed we may required additional fixes. The async processing capability should work within aws-serverless-java-container-core but with the proposed change you made calling filters exclusive to -spring and -springboot3. Filters for apps running -jersey and other potential implementations would no longer be called or am I missing something?

You seem to be correct based on what I've reviewed. I will take a closer look into this

@deki
Copy link
Collaborator

deki commented Jan 12, 2024

@2012160085 just a heads up, we are planning to release 2.0.0 at the end of this month. are you still working on it so we can include it?

@2012160085
Copy link
Contributor Author

@2012160085 just a heads up, we are planning to release 2.0.0 at the end of this month. are you still working on it so we can include it?

I haven't been able to focus on this recently, but I will make sure to wrap it up by the end of the month.

@2012160085
Copy link
Contributor Author

2012160085 commented Jan 21, 2024

@2012160085 just a heads up, we are planning to release 2.0.0 at the end of this month. are you still working on it so we can include it?

@deki I think I've wrapped it up.

  • Modified the serverless-java-container-core to revise the ReDispatch logic.
    I relocated the ReDispatch logic from springboot3 and spring to the serverless-java-container-core, ensuring that the ReDispatch logic now operates exclusively within the serverless-java-container-core.
  • Added JpaAppTest to serverless-java-container-springboot3, which was initially failing with JPA dependencies but now succeeds after my modifications. Additionally, I added asynchronous request test cases to Spring.

@2012160085 2012160085 requested a review from deki January 21, 2024 16:39
@deki deki added this to the Release 2.0 milestone Jan 24, 2024
@deki deki merged commit 69883f6 into aws:main Jan 30, 2024
4 checks passed
@deki
Copy link
Collaborator

deki commented Jan 30, 2024

Thanks again for your contribution, @2012160085!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants