Fix issues with recursive loading #1394
Conversation
This test causes Guice to violate the requirements of Guava's LoadingCache by triggering a recursive load. The recursive load crashes either with an UncheckedExecutionException (bad) or causes a deadlock (terrible). google#785
| return errors.hasErrors() ? errors : result; | ||
| } | ||
| }); | ||
| private final Map<K, FutureTask<V>> delegate = new LinkedHashMap<>(); |
There was a problem hiding this comment.
"curious why this needs to be a linked map instead of just a plain map. if it doesn't need to be linked, we could get rid of explicit synchronization using ConcurrentHashMap instead AFAICT.
(only question of switching to concurrent would be atomicity of calling create(..) during the computeIfAbsent->newFutureTask. unsure if atomic, unsure if needs to be.)"
| futureTask.run(); | ||
| result = futureTask.get(); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
"add comments explaining why we rethrow here (and the non-ErrorsException instanceof below)?"
| if (futureTask.isDone()) { | ||
| result.put(entry.getKey(), futureTask.get()); | ||
| } | ||
| } catch (Exception ignored) { |
There was a problem hiding this comment.
"doc why its safe to ignore the exception -- namely, it was already handled above in get(). that said, it might be nice to switch this back to the old style where it returned V|Errors, which will get rid of a lot of exception handling. it'll introduce some more casting, but IMO ditching the exception handling will end up with cleaner code even w/ the casting. up to you."
| futureTask = delegate.computeIfAbsent(key, this::newFutureTask); | ||
| } | ||
|
|
||
| futureTask.run(); |
There was a problem hiding this comment.
It looks like multiple concurrent threads could attempt to run the same futuretask. Is it what you intended?
Thypically only one thread should run, and the rest wait on .get
|
I’m reviewing old PRs and I abandoned this one before I pushed it over the finish line. That’s my fault. As an alternative, please consider reviewing and merging #1389. That PR demonstrates the bug without a specified fix. Y’all can figure out what fix you like best! |
…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
…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: 525219839
Crash with nice Guice errors rather than Guava crashes or deadlocks.