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

Response headers aren't merged across responding threads #99927

Open
dnhatn opened this issue Sep 26, 2023 · 4 comments
Open

Response headers aren't merged across responding threads #99927

dnhatn opened this issue Sep 26, 2023 · 4 comments
Labels
>bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team

Comments

@dnhatn
Copy link
Member

dnhatn commented Sep 26, 2023

This is related to #99926.

It seems that our infrastructure does not merge response headers across multiple asynchronous calls. Response headers are not merged properly in this scenario:

  1. The caller initiates two asynchronous calls, c1 and c2, which can involve network requests.

  2. c1 responded with a warning in the header responses. We merge these response headers with the original ThreadContext of the calling thread and update the ThreadContext of the current thread while leaving the ThreadContext of the calling thread untouched.

  3. c2 responded with no warning in the header responses. Since the original ThreadContext of the calling thread did not get updated after c1, as it's immutable, we won't be able to merge response headers between c1 and c2.

  4. The caller receives a response from the responding thread of c2 without any warning

@dnhatn dnhatn added >bug :Core/Infra/Core Core issues without another label labels Sep 26, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Sep 26, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@dnhatn
Copy link
Member Author

dnhatn commented Sep 27, 2023

I've thought about this issue a bit more. I think the task framework can provide a solution without requiring changes in other places. The parent task should collect and merge response headers from its child tasks. While it may not be a complete solution, it's an appealing option due to its contained nature. I'm tagging the @elastic/es-distributed team for their awareness.

@ywangd
Copy link
Member

ywangd commented Sep 28, 2023

It seems to me that threadContext management is an explicit choice of the caller. The ThreadContext class provides an array of methods so that request and response headers can be preserved or dropped. Therefore baking response header merging into the task framework seems to be too coarse grained. Would either GroupedActionListener or RefCountingListener be a better place for having this behaviour? My thinking is that these classes can be instantiated with an extra parameter to indicate how response headers should be handled across its multiple threads.

@DaveCTurner
Copy link
Contributor

I was wondering if it should be even lower-level: anywhere that we fan-in responses could potentially need to combine response headers from different thread contexts. There's various fundamental fan-in mechanisms, including CountDown and AbstractRefCounted.

dnhatn added a commit that referenced this issue Dec 6, 2023
We have implemented #99927 in DriverRunner. However, we also need to 
implement this in ComputeService, where we spawn multiple requests to
avoid losing response headers.

Relates #99927

Closes #100163
Closes #102982
Closes #102871
Closes #103028
dnhatn added a commit to dnhatn/elasticsearch that referenced this issue Dec 6, 2023
We have implemented elastic#99927 in DriverRunner. However, we also need to
implement this in ComputeService, where we spawn multiple requests to
avoid losing response headers.

Relates elastic#99927

Closes elastic#100163
Closes elastic#102982
Closes elastic#102871
Closes elastic#103028
elasticsearchmachine pushed a commit that referenced this issue Dec 6, 2023
We have implemented #99927 in DriverRunner. However, we also need to
implement this in ComputeService, where we spawn multiple requests to
avoid losing response headers.

Relates #99927

Closes #100163
Closes #102982
Closes #102871
Closes #103028
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

4 participants