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

SOLR-16840: Workaround HK2 DI bug #1726

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

gerlowskija
Copy link
Contributor

@gerlowskija gerlowskija commented Jun 27, 2023

https://issues.apache.org/jira/browse/SOLR-16840

Description

Prior to this commit many v2 invocations would succeed but log scary looking stacktraces to Solr's console log. This was because of a bug in how Jersey creates and cleans up "RequestScoped" objects. (See Jersey#3503 for more details.)

Solution

This commit works around the bug by changing how several "factory" classes obtain a "ContainerRequestContext": switching away from DI and towards looking up the CRC more explicitly using a "ServiceLocator".

Tests

Manual testing to verify that the warning stacktraces no longer appear in Solr's console logging. Automated tests continue to pass.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.

Prior to this commit many v2 invocations would succeed but log scary
looking stacktraces to Solr's console log.  This was because of a bug in
how Jersey creates and cleans up "RequestScoped" objects.  (See
Jersey#3503 for more details.)

This commit works around the bug by changing how several "factory"
classes obtain a "ContainerRequestContext": switching away from DI and
towards looking up the CRC more explicitly using a "ServiceLocator".
Copy link
Contributor

@epugh epugh left a comment

Choose a reason for hiding this comment

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

LGTM. Finally learned what the hk2 prefix is. https://en.wikipedia.org/wiki/GlassFish_HK2

Just out of curiosity, are we behind on our version of Glassfish and HK2? I.e, are we establishing a pattern of not upgrading and then having to work around things? If/when the bug in HK2 gets fixed, how will we know to eliminate our workaround? DOn't we sometimes annoate code with a bug url?

@HoustonPutman
Copy link
Contributor

So the Github Issue that Jason linked has been open for 6 years, and not closed. So definitely not an upgrade issue.

Yeah we can probably link the Issue url in a comment somewhere, so this is documented in the code. Good idea

@gerlowskija
Copy link
Contributor Author

are we behind on our version of Glassfish and HK2?

No, we're not behind on hk2 - it's just a long standing bug I guess. I've added a comment as you guy suggested to we can use more conventional injection when the bug does eventually get fixed.

@gerlowskija gerlowskija merged commit 541380c into apache:main Jun 30, 2023
1 of 3 checks passed
@gerlowskija gerlowskija deleted the SOLR-16840-fix-jersey-di-error branch June 30, 2023 18:58
gerlowskija added a commit that referenced this pull request Jul 5, 2023
Prior to this commit many v2 invocations would succeed but log scary
looking stacktraces to Solr's console log.  This was because of a bug in
how Jersey creates and cleans up "RequestScoped" objects.  (See
Jersey#3503 for more details.)

This commit works around the bug by changing how several "factory"
classes obtain a "ContainerRequestContext": switching away from DI and
towards looking up the CRC more explicitly using a "ServiceLocator".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants