Skip to content

removed synchronised from code base as much as possible for VT #3317

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
dondonz merged 2 commits into from
Oct 17, 2023
Merged

removed synchronised from code base as much as possible for VT #3317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2c62f1
removed syncrhnoised from code base as much as possible for VT
bbakerman Sep 1, 2023
a602556
better code layout
bbakerman Sep 1, 2023
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
17 changes: 10 additions & 7 deletions src/main/java/graphql/execution/ExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import graphql.normalized.ExecutableNormalizedOperationFactory;
import graphql.schema.GraphQLSchema;
import graphql.util.FpKit;
import graphql.util.LockKit;
import org.dataloader.DataLoaderRegistry;

import java.util.HashSet;
Expand Down Expand Up @@ -49,6 +50,7 @@ public class ExecutionContext {
private final Object localContext;
private final Instrumentation instrumentation;
private final AtomicReference<ImmutableList<GraphQLError>> errors = new AtomicReference<>(ImmutableKit.emptyList());
private final LockKit.ReentrantLock errorsLock = new LockKit.ReentrantLock();
private final Set<ResultPath> errorPaths = new HashSet<>();
private final DataLoaderRegistry dataLoaderRegistry;
private final Locale locale;
Expand Down Expand Up @@ -130,6 +132,7 @@ public CoercedVariables getCoercedVariables() {

/**
* @param <T> for two
*
* @return the legacy context
*
* @deprecated use {@link #getGraphQLContext()} instead
Expand Down Expand Up @@ -178,7 +181,7 @@ public ValueUnboxer getValueUnboxer() {
* @param fieldPath the field path to put it under
*/
public void addError(GraphQLError error, ResultPath fieldPath) {
synchronized (this) {
errorsLock.runLocked(() -> {
Copy link

@He-Pin He-Pin Sep 2, 2023

Choose a reason for hiding this comment

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

I like this kind of change, diff is small

//
// see https://spec.graphql.org/October2021/#sec-Handling-Field-Errors about how per
// field errors should be handled - ie only once per field if it's already there for nullability
Expand All @@ -188,7 +191,7 @@ public void addError(GraphQLError error, ResultPath fieldPath) {
return;
}
this.errors.set(ImmutableKit.addToList(this.errors.get(), error));
}
});
}

/**
Expand All @@ -198,7 +201,7 @@ public void addError(GraphQLError error, ResultPath fieldPath) {
* @param error the error to add
*/
public void addError(GraphQLError error) {
synchronized (this) {
errorsLock.runLocked(() -> {
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
// on how exactly multiple errors should be handled - ie only once per field or not outside the nullability
// aspect.
Expand All @@ -207,7 +210,7 @@ public void addError(GraphQLError error) {
this.errorPaths.add(path);
}
this.errors.set(ImmutableKit.addToList(this.errors.get(), error));
}
});
}

/**
Expand All @@ -220,9 +223,9 @@ public void addErrors(List<GraphQLError> errors) {
if (errors.isEmpty()) {
return;
}
// we are synchronised because we set two fields at once - but we only ever read one of them later
// we are locked because we set two fields at once - but we only ever read one of them later
// in getErrors so no need for synchronised there.
synchronized (this) {
errorsLock.runLocked(() -> {
Set<ResultPath> newErrorPaths = new HashSet<>();
for (GraphQLError error : errors) {
// see https://github.com/graphql-java/graphql-java/issues/888 on how the spec is unclear
Expand All @@ -235,7 +238,7 @@ public void addErrors(List<GraphQLError> errors) {
}
this.errorPaths.addAll(newErrorPaths);
this.errors.set(ImmutableKit.concatLists(this.errors.get(), errors));
}
});
}

/**
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/graphql/execution/directives/QueryDirectivesImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import graphql.schema.GraphQLDirective;
import graphql.schema.GraphQLSchema;
import graphql.util.FpKit;
import graphql.util.LockKit;

import java.util.ArrayList;
import java.util.LinkedHashMap;
Expand All @@ -35,6 +36,8 @@ public class QueryDirectivesImpl implements QueryDirectives {
private final Map<String, Object> variables;
private final GraphQLContext graphQLContext;
private final Locale locale;

private final LockKit.ComputedOnce computedOnce = new LockKit.ComputedOnce();
private volatile ImmutableMap<Field, List<GraphQLDirective>> fieldDirectivesByField;
private volatile ImmutableMap<String, List<GraphQLDirective>> fieldDirectivesByName;
private volatile ImmutableMap<Field, List<QueryAppliedDirective>> fieldAppliedDirectivesByField;
Expand All @@ -49,10 +52,7 @@ public QueryDirectivesImpl(MergedField mergedField, GraphQLSchema schema, Map<St
}

private void computeValuesLazily() {
synchronized (this) {
if (fieldDirectivesByField != null) {
return;
}
computedOnce.runOnce(() -> {

final Map<Field, List<GraphQLDirective>> byField = new LinkedHashMap<>();
final Map<Field, List<QueryAppliedDirective>> byFieldApplied = new LinkedHashMap<>();
Expand Down Expand Up @@ -81,7 +81,7 @@ private void computeValuesLazily() {
this.fieldDirectivesByField = ImmutableMap.copyOf(byField);
this.fieldAppliedDirectivesByName = ImmutableMap.copyOf(byNameApplied);
this.fieldAppliedDirectivesByField = ImmutableMap.copyOf(byFieldApplied);
}
});
}

private QueryAppliedDirective toAppliedDirective(GraphQLDirective directive) {
Expand Down
30 changes: 15 additions & 15 deletions src/main/java/graphql/execution/instrumentation/dataloader/FieldLevelTrackingApproach.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import graphql.execution.instrumentation.InstrumentationState;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.util.LockKit;
import org.dataloader.DataLoaderRegistry;
import org.slf4j.Logger;

Expand All @@ -29,6 +30,8 @@ public class FieldLevelTrackingApproach {

private static class CallStack implements InstrumentationState {

private final LockKit.ReentrantLock lock = new LockKit.ReentrantLock();

private final LevelMap expectedFetchCountPerLevel = new LevelMap();
private final LevelMap fetchCountPerLevel = new LevelMap();
private final LevelMap expectedStrategyCallsPerLevel = new LevelMap();
Expand Down Expand Up @@ -124,10 +127,10 @@ ExecutionStrategyInstrumentationContext beginExecutionStrategy(InstrumentationEx
int parentLevel = path.getLevel();
int curLevel = parentLevel + 1;
int fieldCount = parameters.getExecutionStrategyParameters().getFields().size();
synchronized (callStack) {
callStack.lock.runLocked(() -> {
callStack.increaseExpectedFetchCount(curLevel, fieldCount);
callStack.increaseHappenedStrategyCalls(curLevel);
}
});

return new ExecutionStrategyInstrumentationContext() {
@Override
Expand All @@ -142,20 +145,19 @@ public void onCompleted(ExecutionResult result, Throwable t) {

@Override
public void onFieldValuesInfo(List<FieldValueInfo> fieldValueInfoList) {
boolean dispatchNeeded;
synchronized (callStack) {
dispatchNeeded = handleOnFieldValuesInfo(fieldValueInfoList, callStack, curLevel);
}
boolean dispatchNeeded = callStack.lock.callLocked(() ->
handleOnFieldValuesInfo(fieldValueInfoList, callStack, curLevel)
);
if (dispatchNeeded) {
dispatch();
}
}

@Override
public void onFieldValuesException() {
synchronized (callStack) {
callStack.increaseHappenedOnFieldValueCalls(curLevel);
}
callStack.lock.runLocked(() ->
callStack.increaseHappenedOnFieldValueCalls(curLevel)
);
}
};
}
Expand Down Expand Up @@ -187,19 +189,17 @@ public InstrumentationContext<Object> beginFieldFetch(InstrumentationFieldFetchP
CallStack callStack = (CallStack) rawState;
ResultPath path = parameters.getEnvironment().getExecutionStepInfo().getPath();
int level = path.getLevel();
return new InstrumentationContext<Object>() {
return new InstrumentationContext<>() {

@Override
public void onDispatched(CompletableFuture<Object> result) {
boolean dispatchNeeded;
synchronized (callStack) {
boolean dispatchNeeded = callStack.lock.callLocked(() -> {
callStack.increaseFetchCount(level);
dispatchNeeded = dispatchIfNeeded(callStack, level);
}
return dispatchIfNeeded(callStack, level);
});
if (dispatchNeeded) {
dispatch();
}

}

@Override
Expand Down
18 changes: 10 additions & 8 deletions src/main/java/graphql/execution/reactive/CompletionStageMappingPublisher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package graphql.execution.reactive;

import graphql.Internal;
import graphql.util.LockKit;
import org.reactivestreams.Publisher;
import org.reactivestreams.Subscriber;
import org.reactivestreams.Subscription;
Expand Down Expand Up @@ -44,6 +45,7 @@ public void subscribe(Subscriber<? super D> downstreamSubscriber) {

/**
* Get instance of an upstreamPublisher
*
* @return upstream instance of {@link Publisher}
*/
public Publisher<U> getUpstreamPublisher() {
Expand All @@ -56,6 +58,7 @@ public class CompletionStageSubscriber implements Subscriber<U> {
private final Subscriber<? super D> downstreamSubscriber;
Subscription delegatingSubscription;
final Queue<CompletionStage<?>> inFlightDataQ;
final LockKit.ReentrantLock lock = new LockKit.ReentrantLock();
final AtomicReference<Runnable> onCompleteOrErrorRun;
final AtomicBoolean onCompleteOrErrorRunCalled;

Expand Down Expand Up @@ -137,6 +140,7 @@ public void onComplete() {

/**
* Get instance of downstream subscriber
*
* @return {@link Subscriber}
*/
public Subscriber<? super D> getDownstreamSubscriber() {
Expand All @@ -153,23 +157,21 @@ private void onCompleteOrError(Runnable doneCodeToRun) {
}

private void offerToInFlightQ(CompletionStage<?> completionStage) {
synchronized (inFlightDataQ) {
inFlightDataQ.offer(completionStage);
}
lock.runLocked(() ->
inFlightDataQ.offer(completionStage)
);
}

private boolean removeFromInFlightQAndCheckIfEmpty(CompletionStage<?> completionStage) {
// uncontested locks in java are cheap - we don't expect much contention here
synchronized (inFlightDataQ) {
return lock.callLocked(() -> {
inFlightDataQ.remove(completionStage);
return inFlightDataQ.isEmpty();
}
});
}

private boolean inFlightQIsEmpty() {
synchronized (inFlightDataQ) {
return inFlightDataQ.isEmpty();
}
return lock.callLocked(inFlightDataQ::isEmpty);
}
}
}
20 changes: 14 additions & 6 deletions src/main/java/graphql/parser/MultiSourceReader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import graphql.Assert;
import graphql.PublicApi;
import graphql.util.LockKit;

import java.io.IOException;
import java.io.LineNumberReader;
Expand All @@ -27,6 +28,7 @@ public class MultiSourceReader extends Reader {
private int currentIndex = 0;
private int overallLineNumber = 0;
private final boolean trackData;
private final LockKit.ReentrantLock readerLock = new LockKit.ReentrantLock();


private MultiSourceReader(Builder builder) {
Expand All @@ -37,7 +39,8 @@ private MultiSourceReader(Builder builder) {
@Override
public int read(char[] cbuf, int off, int len) throws IOException {
while (true) {
synchronized (this) {
readerLock.lock();
try {
if (currentIndex >= sourceParts.size()) {
return -1;
}
Expand All @@ -50,6 +53,8 @@ public int read(char[] cbuf, int off, int len) throws IOException {
trackData(cbuf, off, read);
return read;
}
} finally {
readerLock.unlock();
}
}
}
Expand Down Expand Up @@ -147,30 +152,30 @@ public SourceAndLine getSourceAndLineFromOverallLine(int overallLineNumber) {
* @return the line number of the current source. This is zeroes based like {@link java.io.LineNumberReader#getLineNumber()}
*/
public int getLineNumber() {
synchronized (this) {
return readerLock.callLocked(() -> {
if (sourceParts.isEmpty()) {
return 0;
}
if (currentIndex >= sourceParts.size()) {
return sourceParts.get(sourceParts.size() - 1).lineReader.getLineNumber();
}
return sourceParts.get(currentIndex).lineReader.getLineNumber();
}
});
}

/**
* @return The name of the current source
*/
public String getSourceName() {
synchronized (this) {
return readerLock.callLocked(() -> {
if (sourceParts.isEmpty()) {
return null;
}
if (currentIndex >= sourceParts.size()) {
return sourceParts.get(sourceParts.size() - 1).sourceName;
}
return sourceParts.get(currentIndex).sourceName;
}
});
}

/**
Expand Down Expand Up @@ -198,13 +203,16 @@ public List<String> getData() {

@Override
public void close() throws IOException {
synchronized (this) {
readerLock.lock();
try {
for (SourcePart sourcePart : sourceParts) {
if (!sourcePart.closed) {
sourcePart.lineReader.close();
sourcePart.closed = true;
}
}
} finally {
readerLock.unlock();
}
}

Expand Down
Loading