| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
| clearCacheKeys.add(key); | ||
| } | ||
| } else { | ||
| future.complete(value); |
There was a problem hiding this comment.
This misses the Try path of completion
Sorry, something went wrong.
|
@fxbonnet I am not sure about this PR: with the new hook introduced by @bbakerman in #275 you can already offload every completion onto a new thread and hence it will run in parallel if you choose todo so. I have problems seeing how this approach is different/better. |
Sorry, something went wrong.
…potentially in parallel
|
@andimarek I created this PR after my comment on @bbakerman 's PR #275 (comment) |
Sorry, something went wrong.
| } | ||
|
|
||
| List<K> clearCacheKeys = new ArrayList<>(); | ||
| Collection<K> clearCacheKeys = new ConcurrentLinkedQueue<>(); |
There was a problem hiding this comment.
clearCacheKeys may now be accessed concurrently by multiple thread if we execute the Runnables in parallel.
Sorry, something went wrong.
There was a problem hiding this comment.
good call - price of side effects outside a thread
Sorry, something went wrong.
There was a problem hiding this comment.
Requesting changes for a semantic regression around ValueCache hits and context-aware batch loaders.
I opened #278 as a small draft PR showing one possible fix: #278
The PR now constructs a BatchLoaderEnvironment before value-cache filtering:
BatchLoaderEnvironment environment = mkBatchLoaderEnv(keys, keyContexts);
CompletableFuture<List<V>> batchLoad = invokeLoader(environment, keys, keyContexts, queuedFutures, loaderOptions.cachingEnabled());But invokeLoader(..., cachingEnabled=true) can then remove value-cache hits and invoke the actual batch loader with only missedKeys / missedKeyContexts while still passing the original environment:
CompletableFuture<List<V>> batchLoad = invokeLoader(environment, missedKeys, missedKeyContexts, missedQueuedFutures);That breaks the existing semantics of BatchLoaderEnvironment: environment.getKeyContextsList() is expected to be aligned with the keys argument passed to the loader. A concrete failure case:
That means a BatchLoaderWithContext implementation that indexes environment.getKeyContextsList() alongside its keys argument can load B using A's context. This is observable behavior and can be a security/data-isolation issue if key contexts carry tenant, auth, locale, or request-scoped metadata.
Please build the BatchLoaderEnvironment from the actual key/context list passed to the concrete loader call after value-cache filtering, or otherwise preserve the invariant that environment.getKeyContextsList() is ordered and sized to match the loader's keys argument. This should also get a regression test with a partial ValueCache hit and a context-aware batch loader asserting that the missed key receives its own context.
PR #278 does exactly that by rebuilding the environment for missedKeys / missedKeyContexts before the concrete loader invocation and adding the regression test.
Sorry, something went wrong.
|
I opened a small draft PR with one possible fix for the ValueCache / BatchLoaderEnvironment context alignment issue: #278 |
Sorry, something went wrong.
|
Thanks @andimarek I have added your commit to the PR |
Sorry, something went wrong.
|
This should now be ready - it has the extra fix @andimarek wanted |
Sorry, something went wrong.
|
Hi @andimarek is there something I need to do for this PR to be ready to merge? |
Sorry, something went wrong.
|
Hello, this pull request has been inactive for 60 days, so we're marking it as stale. If you would like to continue working on this pull request, please make an update within the next 30 days, or we'll close the pull request. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
When a batch loading function returns, we currently complete all CompletableFutures sequentially on the calling thread. Since CompletableFuture.complete(...) also executes any continuations attached via thenApply(...) or thenCompose(...), large batches can significantly delay the return of dispatch().
This PR introduces an option to execute these continuations in parallel instead.