test(showcase): Add gRPC PQC Showcase integration tests - #13969
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates dependency versions for gRPC, Gson, and Error Prone across several pom.xml files. It also introduces a new integration test, testGrpcPqc_withTls, in ITPostQuantumCryptography.java to verify Post-Quantum Cryptography (PQC) TLS negotiation for gRPC clients. Feedback on the changes highlights a potential data race and visibility issue in the custom gRPC client interceptor used in the test, where capturedMetadata is mutated on the gRPC transport thread and read on the main test thread without synchronization. A suggestion is provided to make the field volatile and safely publish a copy of the metadata.
| private static class GrpcTlsCapturingClientInterceptor implements ClientInterceptor { | ||
| final Metadata capturedMetadata = new Metadata(); | ||
|
|
||
| @Override | ||
| public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall( | ||
| MethodDescriptor<ReqT, RespT> method, | ||
| CallOptions callOptions, | ||
| Channel next) { | ||
| return new ForwardingClientCall.SimpleForwardingClientCall<ReqT, RespT>( | ||
| next.newCall(method, callOptions)) { | ||
| @Override | ||
| public void start(Listener<RespT> responseListener, Metadata headers) { | ||
| super.start( | ||
| new ForwardingClientCallListener.SimpleForwardingClientCallListener<RespT>( | ||
| responseListener) { | ||
| @Override | ||
| public void onHeaders(Metadata headers) { | ||
| capturedMetadata.merge(headers); | ||
| super.onHeaders(headers); | ||
| } | ||
| }, | ||
| headers); | ||
| } | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The capturedMetadata field is mutated on the gRPC transport thread inside onHeaders but read on the main test thread. Since Metadata is not thread-safe and there is no memory barrier or synchronization between these threads, this creates a data race and visibility issue, which can lead to flaky tests.
To ensure thread safety and proper visibility under the Java Memory Model (JMM), declare capturedMetadata as volatile and assign a merged copy of the headers to it inside onHeaders to safely publish the captured metadata.
private static class GrpcTlsCapturingClientInterceptor implements ClientInterceptor {
private volatile Metadata capturedMetadata;
@Override
public <ReqT, RespT> ClientCall<ReqT, RespT> interceptCall(
MethodDescriptor<ReqT, RespT> method,
CallOptions callOptions,
Channel next) {
return new ForwardingClientCall.SimpleForwardingClientCall<ReqT, RespT>(
next.newCall(method, callOptions)) {
@Override
public void start(Listener<RespT> responseListener, Metadata headers) {
super.start(
new ForwardingClientCallListener.SimpleForwardingClientCallListener<RespT>(
responseListener) {
@Override
public void onHeaders(Metadata headers) {
Metadata copy = new Metadata();
copy.merge(headers);
capturedMetadata = copy;
super.onHeaders(headers);
}
},
headers);
}
};
}
}
7f29ebf to
c276c35
Compare
…ion tests - Upgrade <grpc.version> from 1.82.2 to 1.83.0 in gapic-generator-java-pom-parent and grpc-gcp-java. - Upgrade <gson.version> to 2.14.0 and <errorprone.version> to 2.50.0 to satisfy Maven Enforcer rules for gRPC 1.83.0. - Add testGrpcPqc_withTls Showcase TLS integration test in ITPostQuantumCryptography.java using built-in default gRPC transport, configured to trust the Showcase TLS server certificate directly. - Include concise Javadoc documenting gRPC 1.83.0 built-in PQC behavior and explaining the purpose of GrpcTlsCapturingClientInterceptor and HTTP/2 metadata helper methods. - Simplify GrpcTlsCapturingClientInterceptor to only capture initial response headers (onHeaders). - Assert negotiated group is equal to EXPECTED_PQC_GROUP for gRPC without asserting a supported groups list. - Leave all HTTP/JSON tests and setUp() 100% untouched. - Exclude all mTLS test resources, keys, certs, and mTLS logic. TAG=agy CONV=385b9ab5-874c-4c9a-b331-66dab51fef61
c276c35 to
16c429d
Compare
|
|
1 participant
Will be blocked until we can merge gRPC v1.83.0