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

[Performance Bug] HeaderHelper.getSafeFromHeader uses ThreadContext.getHeaders() causing major performance overhead #2757

Closed
parasjain1 opened this issue May 10, 2023 · 1 comment · Fixed by #2768
Labels
bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized

Comments

@parasjain1
Copy link
Contributor

parasjain1 commented May 10, 2023

What is the bug?
Within HeaderHelper.deserializeSafeFromHeader we make a call to HeaderHelper.getSafeFromHeader which “safely” retrieves a header value from ThreadContext. For retrieving a header value for given header name ThreadContext.getHeaders() is being called which creates a new map for all existing headers and returns the same. This creation of new map every time getSafeFromHeader is called causes a major overhead, especially for search use cases.

What is the expected behavior?
Retrieve the header value from ThreadContext without creating a new map

Additional Context & Proposed Solution
Instead of using ThreadContext.getHeaders() we could use ThreadContext.getHeader(key).

One caveat is that ThreadContext.getHeader(key) will return the current header value if present, else the default value. If default value is not present, it’ll return a null value.

While in HeaderHelper.getSafeFromHeader we expect to retrieve the current header value OR null (not the default value).

I'm proposing the following changes to fix this -

Within OpenSearch -

  • Add a method ThreadContext.getHeaderOrDefault(key, defaultVal) which will return the current header value if present, defaultVal otherwise.
  • Optionally, can also add a ThreadContext.isHeaderPresent(key) method which returns true if header is present, false otherwise.

Within Security Plugin -

  • Replace call to ThreadContext.getHeaders() with ThreadContext.getHeaderOrDefault(headerName, null) to avoid unnecessarily creating an additional map

What is your host/environment?

  • OS1.3
  • Security Plugin
@parasjain1 parasjain1 added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels May 10, 2023
@parasjain1
Copy link
Contributor Author

ThreadContext.getHeaders() too returns defaultHeaders when headers are absent in the current requestHeaders. Hence, we can just safely replace ThreadContext.getHeaders() with ThreadContext.getHeader() without any other change needed in ThreadContext.

Following from above, the only change needed is -

Within Security Plugin -

  • Replace call to ThreadContext.getHeaders() with ThreadContext.getHeaderOrDefault(headerName, null) to avoid unnecessarily creating an additional map

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized
Projects
None yet
1 participant