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

Prevent deadlock on "waitForValue" #1633

Closed
wants to merge 1 commit into from
Closed

Prevent deadlock on "waitForValue" #1633

wants to merge 1 commit into from

Conversation

PaulFridrick
Copy link

I tried a workaround to prevent the deadlock on "waitForValue" in the Guava Cache used for the ConstructorInjectorStore (issue #785).

  • I no longer have the unhelpful stacktrace " java.lang.IllegalStateException: Recursive load"
  • In some cases, the IllegalStateException was nether thrown and the thread just hung : this case is not fixed with my workaround (I was not able to reproduce the problem with unit tests)
  • When writing the new unit tests I noticed another issue which is likely related to the root cause of the deadlock : in the cases where the IllegalStateException would occur we are able to get some bindings which Guice should not be able to create (see the added class test)...

I don't think this workaround should be merged as-is, but I hope it will help people find the proper fix...

I will try to dig further if I find the time.

@google-cla
Copy link

google-cla bot commented Jun 2, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@PaulFridrick
Copy link
Author

So I improved the workaround :

  • we should no longer have the unhelpful stacktrace "java.lang.IllegalStateException: Recursive load"
  • it should no longer be possible for the thread to hang
  • the new tests are all green

copybara-service bot pushed a commit that referenced this pull request Apr 18, 2023
…nd/or crash inside Guava's LoadingCache. The problem had to do with the fact that Guice eagerly stores uninitialized JIT bindings in an attempt to allow circular dependencies, and Guice also attempts to cleanup failed JIT bindings (to remove these uninitialized JIT bindings). The JIT binding cache was a guard against a recursive call back into the constructor's cache, but we were removing that guard.

We now only remove the guard if we aren't in the process of loading that JIT binding. The failed JIT binding is naturally cleaned up later on, as the existing & new tests attest to.

Fixes many issues:
 - Fixes #1633
 - Fixes #785
 - Fixes #1389
 - Fixes #1394

Many thanks to @swankjesse for the test-case added in #1389 that was helpful in diagnosing the problem, and to @PaulFridrick for the diagnoses in #1633.

PiperOrigin-RevId: 525135213
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.

1 participant