Skip to content

Add synchronized to loaderQueue inside completable future #89

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

Merged
merged 1 commit into from
Jul 17, 2021
Merged

Add synchronized to loaderQueue inside completable future #89

merged 1 commit into from
Jul 17, 2021

Conversation

prokop7
Copy link
Contributor

@prokop7 prokop7 commented Jul 16, 2021

Describe the bug
A bug that may cause jobs not to be added to loaderQueue due to race conditions.
DataLoader.load(K key, Object loadContext) has two ways of getting value, first - get immediately or add to queue, second - retrieve from the cache. Retrieving from cache works through consecutive CompletableFuture calls, if the cache misses then we add to queue or load immediately. The first way covered by synchronized on dataLoader but the second isn't.

This PR fixes this issue by adding synchronized on callback if the cache misses

To Reproduce
It's hard to reproduce because of multithreading

@bbakerman
Copy link
Member

I am surprised this is working

because that bit of code is only called from one place

    CompletableFuture<V> load(K key, Object loadContext) {
        synchronized (dataLoader) {
            boolean batchingEnabled = loaderOptions.batchingEnabled();
            boolean cachingEnabled = loaderOptions.cachingEnabled();

            stats.incrementLoadCount();

            if (cachingEnabled) {
                return loadFromCache(key, loadContext, batchingEnabled);
            } else {
                return queueOrInvokeLoader(key, loadContext, batchingEnabled);
            }
        }
    }

As you can see its already inside a synchronized call on the data loader above.

Have you tried this on latest master (which was refactored recently)?

@bbakerman
Copy link
Member

Ahhh its during the CF callback. Great catch!

@bbakerman bbakerman merged commit 682c652 into graphql-java:master Jul 17, 2021
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.

2 participants