CallOptions#withOption() mutates original instance if existing key is overwritten

[groakley]

CallOptions should be immutable. However, if CallOptions#withOption() is called with a key that is equivalent to an existing custom option, the original CallOptions instance has its option overwritten.

The following test case reproduces the issue:

@Test
testMutability() {
    final CallOptions.Key<String> OPTION_1
          = CallOptions.Key.createWithDefault("option1", "default");

    CallOptions defaultOpt = CallOptions.DEFAULT;
    CallOptions opt1 = defaultOpt.withOption(OPTION_1, "v1");
    CallOptions opt2 = opt1.withOption(OPTION_1, "v2");

    assertThat(defaultOpt.getOption(OPTION_1)).isEqualTo("default");
    assertThat(opt1.getOption(OPTION_1)).isEqualTo("v1");   // This fails! Is actually "v2"
    assertThat(opt2.getOption(OPTION_1)).isEqualTo("v2");
}

This causes issues when combined with AbstractStub#withOption(), as the stub's option can be permanently overwritten by ClientInterceptors. See this example:

  final CallOptions.Key<String> testKey = CallOptions.Key.create("testKey");

    TestLiteServiceBlockingStub stub =
        TestLiteServiceGrpc.newBlockingStub(grpcServerRule.getChannel()).withOption(testKey, "foo");

    TestResponse response1 =
        stub.withInterceptors(
                new ClientInterceptor() {
                  @Override
                  public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
                      MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
                    assertThat(callOptions.getOption(testKey)).isEqualTo("bar");
                    return next.newCall(method, callOptions);
                  }
                },
                new ClientInterceptor() {
                  @Override
                  public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
                      MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
                    // Override the option from AbstractStub#withOption()
                    return next.newCall(method, callOptions.withOption(testKey, "bar"));
                  }
                })
            .testMethod(TEST_REQUEST);
    assertThat(response1).isEqualTo(TEST_RESPONSE);

    TestResponse response2 =
        stub.withInterceptors(
                new ClientInterceptor() {
                  @Override
                  public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
                      MethodDescriptor<ReqT, RespT> method, CallOptions callOptions, Channel next) {
                    // *** The following assertion fails ***
                    // This should be the value with set using AbstractStub#withOption()
                    assertThat(callOptions.getOption(testKey)).isEqualTo("foo");
                    return next.newCall(method, callOptions);
                  }
                })
            .testMethod(TEST_REQUEST);
    assertThat(response2).isEqualTo(TEST_RESPONSE);

[groakley]

Sent #5143, which fixes the issue.

[creamsoup]

thanks for the fix