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

Fixes to completion of RequestManager in HttpMode #278

Closed
wants to merge 7 commits into from

Conversation

lachlan-roberts
Copy link
Collaborator

For the HttpConnector mode we now do not call RequestManager#finishRequest until we get HttpStream completion in Jetty12, or HttpChannel completion in Jetty-9.4.

@lachlan-roberts
Copy link
Collaborator Author

@gregw @ludoch the error still occurs after these changes, and I can see that for Jetty94, the datastore.put() for DatastoreSessionStore is always called before the requestManager.finishRequest() even in case of the error. And these are also called by the same thread.

So I do not know why we get the error "the API call to datastore_v3.Put() was explicitly cancelled". Because we are not out of the scope of the request because we have not called RequestManager.finishRequest() yet.

@ludoch
Copy link
Collaborator

ludoch commented Sep 27, 2024

Thanks Lachlan for the deep investigation. Its indicates that where the issue is: in the backend C++ api server and request manager, which has a potential of race condition between the end of the request and the 'release' of the API ticket necessary to do the last datastore put to store the session...
I am inclined on trying to fix this race condition on the appserver side, not Java side. In fact, searching this ApiProxy$CancelledException error in our internal bug system shows that it happened a little bit in the past, and then stopped showing up so no actions were taken...

@ludoch
Copy link
Collaborator

ludoch commented Sep 27, 2024

Filed #279

Signed-off-by: Lachlan Roberts <[email protected]>
@ludoch
Copy link
Collaborator

ludoch commented Sep 30, 2024

Working on the server side fix in C++

@ludoch
Copy link
Collaborator

ludoch commented Nov 15, 2024

can we abandon this PR as we now ignore CANCEL error and the C++ code push is going on to remove it?

* Fix HttpServletRequest.getRemoteAddr for the HttpConnector mode

Signed-off-by: Lachlan Roberts <[email protected]>

* assign a static field for "0.0.0.0" string

Signed-off-by: Lachlan Roberts <[email protected]>

---------

Signed-off-by: Lachlan Roberts <[email protected]>
Signed-off-by: Lachlan Roberts <[email protected]>
@lachlan-roberts lachlan-roberts marked this pull request as ready for review November 19, 2024 08:00
@lachlan-roberts
Copy link
Collaborator Author

I think this PR is still good good to have as it prevents RequestManager#finishRequest from being run before the session is written. So will prevent any future issues which could arise from this.

@lachlan-roberts
Copy link
Collaborator Author

closing as it looks like the changes have already been included in main branch

@lachlan-roberts lachlan-roberts deleted the HttpModeRequestManager branch December 9, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTTPConnector mode: Servlet session datastore.put() call to save the session is flaky
3 participants