grpc-java
grpc-java copied to clipboard
grpc
Tracking Issue for Testing utilities being Experimental.
I realized that all members of TestUtils have been deprecated. We use them for our gRPC interop tests:
https://github.com/line/armeria/blob/master/it/src/test/java/com/linecorp/armeria/server/grpc/interop/ArmeriaGrpcServerInteropTest.java#L80
I also see a similar problem on the client side. We need TestUtils.newSslSocketFactoryForCa().
What would be the recommended way to run this sort of interop tests?
@trustin I think you can define/fork your own function similar to TestUtils.newSslSocketFactoryForCa(), it's not hard because the function is not dependent on gRPC library.
The source code is at https://github.com/grpc/grpc-java/blob/master/testing/src/main/java/io/grpc/internal/testing/TestUtils.java
If certain methods are useful, have a reasonable API, and are maintainable, we can keep them.
@trustin, details about your test:
- It is not using GrpcSslContexts. It can override
trustManagerafter configuring the SslContext via GrpcSslContexts, but it should pass through GrpcSslContexts. preferredTestCiphers()really should have been internal. Although you are using it the same as we are (testing OkHttp on the JDK). This was needed when the JDK's GCM went at 2 MB/s. Now it's 10x that and JDK 9 will be 10x again. So maybe we just drop it. Alternatively we can kill it in favor of Conscrypt. It is only helpful with Jetty ALPN. Conscrypt is very close to 1.0 with pre-built binaries for multiple platforms. But OkHttp will need some fixes to make use of it off-Android.- The usage of
newSslSocketFactoryForCais coupled with a usage ofio.grpc.okhttp.internal.Platform. That's obviously not supported. But I understand the need. - There's a usage of
GrpcUtil.authorityFromHostAndPortwhich is also internal. It could have been just"example.com:" + port. A big reason for the utility is to handle IPv6 IPs which need[]. That's unnecessary in the test
I'm fine to work out utility functions for using test certificates. I understand that is a major PITA and using grpc's pre-generated test certs makes things much easier. But I expect they could be simplified.
For example, Netty now has InputStream-based methods for some of these things. loadCert() could actually just return an InputStream. That means loadX509Cert is probably unnecessary now-a-days.
Functionality that looks good to keep (but maybe renamed/moved/tweaked):
- One-line impl for
InputStream loadCert(String name) newSslSocketFactoryForCa()it is useful for OkHttp, but... providing Provider is a problem. Maybe we can add a method to OkHttpChannelBuilder. MaybetrustManagerFactory(TrustManagerFactory)(+ testing utility) ortrustCertificates(InputStream)that is used when we create our SSLContext. That would avoid needing to specify a Provider.
@ejona86 Thanks a lot for the detailed suggestions and ideas. Using GrpcSslContexts simplified things quite a bit. Let me keep the usage of loadCert() and newSslSocketFactoryForCa() for the time being.
Deprecated io.grpc.testing.StreamRecorder in v1.8.0 and to be removed at v1.9.0
Deprecated public static ServerInterceptor recordServerCallInterceptor( final AtomicReference<ServerCall<?, ?>> serverCallCapture) in v1.8.0 and to be removed at v1.9.0
I see that StreamRecorder has been deprecated with a comment:
Most users should use blocking stubs instead.
Wait, how? How do we use a blocking stub in bidirectional streaming calls? That's exactly what we've been using StreamRecorder for. It's a shame that a useful testing utility is going away without any replacement / alternative.
To use properly, you must know the class's implementation; if you liked it, copy the code to your own codebase
We absolutely will copy the class. I don't understand the "you must know the class's implementation" part, though. The class is nicely JavaDoc'ed and we've been using it for quite some time without any knowledge about its implementation. What are the details that we need to know and how can we misuse the class? It seems to be dead simple.
@JanecekPetr wrote:
How do we use a blocking stub in bidirectional streaming calls?
You're right, blocking stubs don't solve client-streaming and bidi-streaming. However, those sorts of methods are rare, so that's why the "most" is there before "most users." And of the remaining, many would use mocks, since mocks are in vogue. The class is also not helpful in many cases of bidi calls since it doesn't let you wait for a response to arrive.
It's actually useful to know you are using it, so pinging this was a "Good Thing." We looked through usages (internally) and only found one user, and that's because they used StreamObserver as part of their API. So having counter evidence is helpful.
What are the details that we need to know and how can we misuse the class?
Most recent thing that came up (#2385) was whether getError() was thread-safe. There's no documentation as to the thread-safety of the class. awaitCompletion() is a good signal that it's at least partially thread-aware, but it should document that you shouldn't call getError() before completion, and what happens if you call getValues() before completion (do you get a snapshot of the values, do the values mutate live, or is it not thread-safe?).
The class isn't really up to our normal standards; it was primarily exposed because it needed to be used in our tests (before blocking stubs, but also with streaming), and grpc-testing was a convenient place for that (in fact, that's what grpc-testing initially was; only utilities we needed for ourselves). So we're trying to clean things up and make it more clear. The class could be improved to be more maintainable: make it a final class and improve documentation. But if nobody was using it, there's not much point.
There's been talk of providing real streaming APIs for blocking, with an API sketch I've seen that looks feasible. Although I realize it may be a pain to migrate your tests, if you were able to do full bidi streaming in a no-need-for-other-threads blocking fashion, would that be a welcome approach for your tests?
@dapengzhang0, since @JanecekPetr noticed the planned removal in less than a workday of the release, it seems unlikely he's the only user. So it'd probably be fair to revert #3790 until we have a good streaming blocking API (or similar answer for the use-case gaps). But we should make a new issue for that and not lump it in here.
if you were able to do full bidi streaming in a no-need-for-other-threads blocking fashion, would that be a welcome approach for your tests?
Yes, thank you, that would be fine. Migrating tests is of course a pain, but one that can be done reasonably easily and fast. Thanks!
... until we have a good streaming blocking API (or similar answer for the use-case gaps). But we should make a new issue for that and not lump it in here.
I guess once #10318 is merged we can delete StreamRecorder? Also io.grpc.testing.TestUtils is still being used so cannot be removed just yet?
