CallOptions is not thread-safe

[kilink]

Although it is annotated with Immutable, CallOptions has no final fields, and is therefore not actually thread-safe. The javadoc for the Immutable annotations states:

The class to which this annotation is applied is immutable. This means that its state cannot be seen to change by callers.

This can be shown to be violated with a simple JCStress test case; here is an example.

When I run the above test case on my aarch64 machine, I get results such as the following, showing memory visibility issues:

       Observed state   Occurrences   Expectation  Interpretation                                              
  -1, -1, not visible   125,492,608    ACCEPTABLE  Object is not seen yet.                                     
    -2, -2, authority            11     FORBIDDEN  Everything else is forbidden.                               
         -2, -2, null             8     FORBIDDEN  Everything else is forbidden.                               
     5, -2, authority             6     FORBIDDEN  Everything else is forbidden.                               
      5, 5, authority     2,139,458    ACCEPTABLE  Seen the complete object.           

Although to be honest, the above test case is not really needed, as one can simply refer to the JLS [1] [2] to see that CallOptions is not safely published and is therefore not guaranteed to be seen as consistent across threads.

The CallOptions class has a comment explaining why the fields are not final, and it seems to be for stylistic reasons. I would think correctness would outweigh such concerns and the fields should be made final, or the Immutable annotation should be removed from the class.

[1] https://shipilev.net/blog/2014/safe-public-construction/#_safe_publication
[2] https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5

[ejona86]

You are sharing the object with unsynchronized publication. I don't think we care at all about unsynchronized publication, as it is generally seen as an antipattern and so mostly matters for security-related objects. But I do see that @Immutable says:

they may be passed between threads or published without synchronization

Since this object is created enough that we don't want to use a separate internal builder just to improve the ease of setting final fields, I think we'd just remove the annotation and use text to say the object is immutable but may not be published without synchronization.

[kilink]

Right, that was my point. @Immutable implies it is safe to share the object without synchronization, regardless of how it's used internally in grpc-java.

Since this object is created enough that we don't want to use a separate internal builder just to improve the ease of setting final fields, I think we'd just remove the annotation and use text to say the object is immutable but may not be published without synchronization.

Couldn't the internal code just "grin and bear it" with a high-arity constructor? It's not as though it's exposed as part of the API. For code that changes infrequently it doesn't seem THAT error prone (e.g. with comments), or is it a matter of it simply be banned by some style guide / convention? It seems to me the fields being final outweighs the ugliness of a high-arity constructor.

[ejona86]

Yeah, the code could just grin and bear it. It is easy to mix up argument ordering and the like, though. And adding a new field becomes annoying. There is also a potential option of trying out a builder and seeing if C2 JIT actually avoids the builder allocation.

It seems to me the fields being final outweighs the ugliness of a high-arity constructor

What is the benefit? I don't think normal usages in gRPC would see any benefit, even for stub objects.

[ejona86]

There is also a potential option of trying out a builder and seeing if C2 JIT actually avoids the builder allocation.

Looks like on OpenJDK the Builder allocation is properly optimized away. That'd probably be true on Android as well, as the code is pretty canonical for "temporary stack object." So I'm fine swapping to an internal builder.

[kilink]

What is the benefit? I don't think normal usages in gRPC would see any benefit, even for stub objects.

I guess the main benefit would be not having to explain in the Javadoc that the class is immutable but not thread-safe, which seems awkward 😄

Looks like on OpenJDK the Builder allocation is properly optimized away. That'd probably be true on Android as well, as the code is pretty canonical for "temporary stack object." So I'm fine swapping to an internal builder.

Great, sounds good.