Skip to content
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

Jump to bottom

Fix issue: CallOptions is not thread-safe. #9689

Merged
merged 9 commits into from
Nov 17, 2022
+92 −68
Merged

Fix issue: CallOptions is not thread-safe. #9689

Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
daee733
Fix issue: CallOptions is not thread-safe.
pandaapo Nov 12, 2022
c8fb1de
Update copyright in file header and adjust indentation.
pandaapo Nov 12, 2022
6f88306
Fix checkstyle error.
pandaapo Nov 12, 2022
dea7008
Reformat code
pandaapo Nov 12, 2022
2e62fea
Modify as reviews.
pandaapo Nov 16, 2022
a523fdb
Modify as review
pandaapo Nov 16, 2022
1315352
Modify as review.
pandaapo Nov 16, 2022
98bfa85
Modify as review
pandaapo Nov 16, 2022
0973147
Modify as review
pandaapo Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
153 changes: 89 additions & 64 deletions api/src/main/java/io/grpc/CallOptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,42 +41,68 @@ public final class CallOptions {
/**
* A blank {@code CallOptions} that all fields are not set.
*/
public static final CallOptions DEFAULT = new CallOptions();
public static final CallOptions DEFAULT = new Builder().build();

// Although {@code CallOptions} is immutable, its fields are not final, so that we can initialize
// them outside of constructor. Otherwise the constructor will have a potentially long list of
// unnamed arguments, which is undesirable.
@Nullable
private Deadline deadline;
private final Deadline deadline;

@Nullable
private Executor executor;
private final Executor executor;

@Nullable
private String authority;
private final String authority;

@Nullable
private CallCredentials credentials;
private final CallCredentials credentials;

@Nullable
private String compressorName;
private final String compressorName;

private Object[][] customOptions;
private final Object[][] customOptions;

// Unmodifiable list
private List<ClientStreamTracer.Factory> streamTracerFactories = Collections.emptyList();
private final List<ClientStreamTracer.Factory> streamTracerFactories;

/**
* Opposite to fail fast.
*/
@Nullable
private Boolean waitForReady;
private final Boolean waitForReady;

@Nullable
private Integer maxInboundMessageSize;
private final Integer maxInboundMessageSize;
@Nullable
private Integer maxOutboundMessageSize;
private final Integer maxOutboundMessageSize;

private CallOptions(Builder builder) {
this.deadline = builder.deadline;
this.executor = builder.executor;
this.authority = builder.authority;
this.credentials = builder.credentials;
this.compressorName = builder.compressorName;
this.customOptions = builder.customOptions;
this.streamTracerFactories = builder.streamTracerFactories;
this.waitForReady = builder.waitForReady;
this.maxInboundMessageSize = builder.maxInboundMessageSize;
this.maxOutboundMessageSize = builder.maxOutboundMessageSize;
}

static class Builder {
Deadline deadline;
Executor executor;
String authority;
CallCredentials credentials;
String compressorName;
Object[][] customOptions = new Object[0][2];
Copy link
Member

Choose a reason for hiding this comment

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

Although this object will only be used once, this will create it all the time. This and the emptyList() below are just for DEFAULT, so we should just make a static method to configure the default builder, or use a static block. I'll do that as a follow-up.

The optimizer should optimize these away, but it would still be there on startup and cause a non-trivial constructor. So it isn't a big deal, but still worth doing.

For reference, it'll look something like this:

static {
  Builder b = new Builder();
  b.customOptions = new Object[0][2];
  b.streamTracerFactories = Collections.emptyList();
  DEFAULT = b.build();
}

// or

public static final CallOptions DEFAULT = newDefault();

private static CallOptions newDefault() {
  Builder b = new Builder();
  b.customOptions = new Object[0][2];
  b.streamTracerFactories = Collections.emptyList();
  return b.build();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The optimizer should optimize these away, but it would still be there on startup and cause a non-trivial constructor.

Thanks for your work. I am curious about this. Does you mean that the java optimizer can optimize Builder to make customOptions will not be created all the time even if without modifying my Builder?

// Unmodifiable list
List<ClientStreamTracer.Factory> streamTracerFactories = Collections.emptyList();
Boolean waitForReady;
Integer maxInboundMessageSize;
Integer maxOutboundMessageSize;

private CallOptions build() {
return new CallOptions(this);
}
}

/**
* Override the HTTP/2 authority the channel claims to be connecting to. <em>This is not
Expand All @@ -89,18 +115,18 @@ public final class CallOptions {
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1767")
public CallOptions withAuthority(@Nullable String authority) {
CallOptions newOptions = new CallOptions(this);
newOptions.authority = authority;
return newOptions;
Builder builder = toBuilder(this);
builder.authority = authority;
return builder.build();
}

/**
* Returns a new {@code CallOptions} with the given call credentials.
*/
public CallOptions withCallCredentials(@Nullable CallCredentials credentials) {
CallOptions newOptions = new CallOptions(this);
newOptions.credentials = credentials;
return newOptions;
Builder builder = toBuilder(this);
builder.credentials = credentials;
return builder.build();
}

/**
Expand All @@ -113,9 +139,9 @@ public CallOptions withCallCredentials(@Nullable CallCredentials credentials) {
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/1704")
public CallOptions withCompression(@Nullable String compressorName) {
CallOptions newOptions = new CallOptions(this);
newOptions.compressorName = compressorName;
return newOptions;
Builder builder = toBuilder(this);
builder.compressorName = compressorName;
return builder.build();
}

/**
Expand All @@ -127,9 +153,9 @@ public CallOptions withCompression(@Nullable String compressorName) {
* @param deadline the deadline or {@code null} for unsetting the deadline.
*/
public CallOptions withDeadline(@Nullable Deadline deadline) {
CallOptions newOptions = new CallOptions(this);
newOptions.deadline = deadline;
return newOptions;
Builder builder = toBuilder(this);
builder.deadline = deadline;
return builder.build();
}

/**
Expand All @@ -156,19 +182,19 @@ public Deadline getDeadline() {
* fails RPCs without sending them if unable to connect.
*/
public CallOptions withWaitForReady() {
CallOptions newOptions = new CallOptions(this);
newOptions.waitForReady = Boolean.TRUE;
return newOptions;
Builder builder = toBuilder(this);
builder.waitForReady = Boolean.TRUE;
return builder.build();
}

/**
* Disables 'wait for ready' feature for the call.
* This method should be rarely used because the default is without 'wait for ready'.
*/
public CallOptions withoutWaitForReady() {
CallOptions newOptions = new CallOptions(this);
newOptions.waitForReady = Boolean.FALSE;
return newOptions;
Builder builder = new Builder();
pandaapo marked this conversation as resolved.
Show resolved Hide resolved
builder.waitForReady = Boolean.FALSE;
return builder.build();
}

/**
Expand Down Expand Up @@ -208,9 +234,9 @@ public CallCredentials getCredentials() {
* executor specified with {@link ManagedChannelBuilder#executor}.
*/
public CallOptions withExecutor(@Nullable Executor executor) {
CallOptions newOptions = new CallOptions(this);
newOptions.executor = executor;
return newOptions;
Builder builder = toBuilder(this);
builder.executor = executor;
return builder.build();
}

/**
Expand All @@ -221,13 +247,13 @@ public CallOptions withExecutor(@Nullable Executor executor) {
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2861")
public CallOptions withStreamTracerFactory(ClientStreamTracer.Factory factory) {
CallOptions newOptions = new CallOptions(this);
ArrayList<ClientStreamTracer.Factory> newList =
new ArrayList<>(streamTracerFactories.size() + 1);
newList.addAll(streamTracerFactories);
newList.add(factory);
newOptions.streamTracerFactories = Collections.unmodifiableList(newList);
return newOptions;
Builder builder = toBuilder(this);
builder.streamTracerFactories = Collections.unmodifiableList(newList);
return builder.build();
}

/**
Expand Down Expand Up @@ -319,7 +345,7 @@ public <T> CallOptions withOption(Key<T> key, T value) {
Preconditions.checkNotNull(key, "key");
Preconditions.checkNotNull(value, "value");

CallOptions newOptions = new CallOptions(this);
Builder builder = toBuilder(this);
int existingIdx = -1;
for (int i = 0; i < customOptions.length; i++) {
if (key.equals(customOptions[i][0])) {
Expand All @@ -328,7 +354,8 @@ public <T> CallOptions withOption(Key<T> key, T value) {
}
}

newOptions.customOptions = new Object[customOptions.length + (existingIdx == -1 ? 1 : 0)][2];
builder.customOptions = new Object[customOptions.length + (existingIdx == -1 ? 1 : 0)][2];
CallOptions newOptions = builder.build();
pandaapo marked this conversation as resolved.
Show resolved Hide resolved
System.arraycopy(customOptions, 0, newOptions.customOptions, 0, customOptions.length);

if (existingIdx == -1) {
Expand Down Expand Up @@ -368,10 +395,6 @@ public Executor getExecutor() {
return executor;
}

private CallOptions() {
customOptions = new Object[0][2];
}

/**
* Returns whether <a href="https://github.com/grpc/grpc/blob/master/doc/wait-for-ready.md">
* 'wait for ready'</a> option is enabled for the call. 'Fail fast' is the default option for gRPC
Expand All @@ -392,9 +415,9 @@ Boolean getWaitForReady() {
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2563")
public CallOptions withMaxInboundMessageSize(int maxSize) {
checkArgument(maxSize >= 0, "invalid maxsize %s", maxSize);
CallOptions newOptions = new CallOptions(this);
newOptions.maxInboundMessageSize = maxSize;
return newOptions;
Builder builder = toBuilder(this);
builder.maxInboundMessageSize = maxSize;
return builder.build();
}

/**
Expand All @@ -403,9 +426,9 @@ public CallOptions withMaxInboundMessageSize(int maxSize) {
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/2563")
public CallOptions withMaxOutboundMessageSize(int maxSize) {
checkArgument(maxSize >= 0, "invalid maxsize %s", maxSize);
CallOptions newOptions = new CallOptions(this);
newOptions.maxOutboundMessageSize = maxSize;
return newOptions;
Builder builder = toBuilder(this);
builder.maxOutboundMessageSize = maxSize;
return builder.build();
}

/**
Expand All @@ -427,19 +450,21 @@ public Integer getMaxOutboundMessageSize() {
}

/**
* Copy constructor.
* Copy CallOptions.
*/
private CallOptions(CallOptions other) {
deadline = other.deadline;
authority = other.authority;
credentials = other.credentials;
executor = other.executor;
compressorName = other.compressorName;
customOptions = other.customOptions;
waitForReady = other.waitForReady;
maxInboundMessageSize = other.maxInboundMessageSize;
maxOutboundMessageSize = other.maxOutboundMessageSize;
streamTracerFactories = other.streamTracerFactories;
private static Builder toBuilder(CallOptions other) {
Builder builder = new Builder();
builder.deadline = other.deadline;
builder.executor = other.executor;
builder.authority = other.authority;
builder.credentials = other.credentials;
builder.compressorName = other.compressorName;
builder.customOptions = other.customOptions;
builder.streamTracerFactories = other.streamTracerFactories;
builder.waitForReady = other.waitForReady;
builder.maxInboundMessageSize = other.maxInboundMessageSize;
builder.maxOutboundMessageSize = other.maxOutboundMessageSize;
return builder;
}

@Override
Expand Down