Skip to content

Add @GuardedBy and remove redundant synchronized #105

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.

Sign up for GitHub

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
Dec 27, 2021
Merged

Add @GuardedBy and remove redundant synchronized #105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/main/java/org/dataloader/DataLoaderHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.dataloader;

import org.dataloader.annotations.GuardedBy;
import org.dataloader.annotations.Internal;
import org.dataloader.impl.CompletableFutureKit;
import org.dataloader.stats.StatisticsCollector;
Expand Down Expand Up @@ -287,6 +288,7 @@ private void possiblyClearCacheEntriesOnExceptions(List<K> keys) {
}
}

@GuardedBy("dataLoader")
private CompletableFuture<V> loadFromCache(K key, Object loadContext, boolean batchingEnabled) {
final Object cacheKey = loadContext == null ? getCacheKey(key) : getCacheKeyWithContext(key, loadContext);

Expand All @@ -296,15 +298,12 @@ private CompletableFuture<V> loadFromCache(K key, Object loadContext, boolean ba
return futureCache.get(cacheKey);
}

CompletableFuture<V> loadCallFuture;
synchronized (dataLoader) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a weird PR because you gave no explanation of why this synchronised call is not needed.

Are you sayin that a synchronized is already in place when this is called??

the loader queue is the key bit of mutable state in the DataLoader (the list of promises to get data at some future batch time) and hence needs synchronization of some sort.

Copy link
Member

Choose a reason for hiding this comment

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

My point is - explanation is important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bbakerman Thanks for your response.

Yes. As @GuardedBy("dataLoader") describe, the thread is already hold dataLoader lock when loadFromCache is called.

java-dataloader/src/main/java/org/dataloader/DataLoaderHelper.java

Line 102 in 4253050

synchronized (dataLoader) {

loadCallFuture = queueOrInvokeLoader(key, loadContext, batchingEnabled, true);
}

CompletableFuture<V> loadCallFuture = queueOrInvokeLoader(key, loadContext, batchingEnabled, true);
futureCache.set(cacheKey, loadCallFuture);
return loadCallFuture;
}

@GuardedBy("dataLoader")
private CompletableFuture<V> queueOrInvokeLoader(K key, Object loadContext, boolean batchingEnabled, boolean cachingEnabled) {
if (batchingEnabled) {
CompletableFuture<V> loadCallFuture = new CompletableFuture<>();
Expand Down
21 changes: 21 additions & 0 deletions src/main/java/org/dataloader/annotations/GuardedBy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package org.dataloader.annotations;

import static java.lang.annotation.ElementType.FIELD;
import static java.lang.annotation.ElementType.METHOD;
import static java.lang.annotation.RetentionPolicy.CLASS;

import java.lang.annotation.Retention;
import java.lang.annotation.Target;

/**
* Indicates that the annotated element should be used only while holding the specified lock.
*/
@Target({FIELD, METHOD})
@Retention(CLASS)
public @interface GuardedBy {

/**
* The lock that should be held.
*/
String value();
}