Fix issue: CallOptions is not thread-safe.

[pandaapo]

Fixes #9658

Although CallOptions is annotated by @Immutable, its fields are not final. So it's not truly immutable, namely not thread-safe.

This PR add final to all fields of CallOptions and remove @Immutable. Using internal builder class to keep flexibility of constructing CallOptions .

[sanjaypujare]

couple of comments added

[sanjaypujare]

Looks good but I would like to see my last comment addressed

[pandaapo]

@sanjaypujare @ejona86 Could you help to merge this PR now?

[sanjaypujare]

@sanjaypujare @ejona86 Could you help to merge this PR now?

Couple of things.

• there is a test failure

The details are here https://source.cloud.google.com/results/invocations/ac617d67-b1bf-41b5-8cb4-3de99df7c53b/targets/github%2Fgrpc-java%2Fapi%2Fbuild%2Ftest-results%2Ftest%2FTEST-io.grpc.CallOptionsTest/tests and this shows up as

expected : 59.980091551s from now
but was : 59.979571156s from now
outside tolerance in ns: 10000000
at io.grpc.CallOptionsTest.withDeadlineAfter(CallOptionsTest.java:141)

Most probably not related to this change but can you look into it?

• @ejona86 needs to approve. I will then merge

[pandaapo]

• there is a test failure

The details are here https://source.cloud.google.com/results/invocations/ac617d67-b1bf-41b5-8cb4-3de99df7c53b/targets/github%2Fgrpc-java%2Fapi%2Fbuild%2Ftest-results%2Ftest%2FTEST-io.grpc.CallOptionsTest/tests and this shows up as

expected : 59.980091551s from now but was : 59.979571156s from now outside tolerance in ns: 10000000 at io.grpc.CallOptionsTest.withDeadlineAfter(CallOptionsTest.java:141)

Most probably not be related to this change but can you look into it?

I noticed this. I want to fix it but my OS is windows. I can not reproduce this failure.

[sanjaypujare]

I noticed this. I want to fix it but my OS is windows. I can not reproduce this failure.

This could just be a flake because of its nature but it happens to be in CallOptionsTest which is the part you touched. Not sure what you were planning to fix.

Let me rerun the tests and see what happens.

[ejona86]

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();
}

[pandaapo]

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?

[ejona86]

@pandaapo, thank you!