Add implementation note regarding server interceptors and thread locals

[ST-DDT]

There seems to be some confusion regarding the correct usage of thread locals inside ServerInterceptors.

• LogNet/grpc-spring-boot-starter (included in the latest official release, see also the related issue)
• revinate/grpc-spring-security-demo
• pagrus7/grpc-spring-boot-starter

In all these examples the thread local is assigned during interceptCall/startCall and only removed on completion/error/close.
I try to warn about these broken implementations, but they are still out there after all and even new ones are created. IMO we should add a hint to the ServerInterceptor that strongly warns about the wrong usage of thread locals inside ServerInterceptors.

I also considered adding such a hint to ServerCall.Listener and the the corresponding client classes, but I would like to ask for opinion first.

[ejona86]

Using a thread local like that is thread-hostile. The StreamObserver documentation tried to set expectations by saying it must be thread-compatible, but it seems ThreadLocal wasn't discussed on the linked page. {Client,Server}Call have "not thread-safe" mention, but don't go into more detail. It seems the Java community uses the terms frequently, but there isn't a good website to link to provide explanation: the details are in things like Java Concurrency in Practice and Effective Java.

I think ClientCall.Listener is probably a better place to document this than ClientInterceptor, although we can make a reminder in the interceptor.

I think the suggested language is too strong and not actually true. A cache stored in a ThreadLocal would be fine. I think we instead need to note that each call may be on a different thread.

I think we should come up with whatever language we want, and include it across StreamObserver, ClientCall+Listener, ServerCall+Listener. We can then have {Server,Client}Interceptor have a brief mention and say to read the {Server,Client}Call documentation.

We might say something like:

Implementations are not guaranteed to be thread-safe, but they must not be thread-hostile. The caller is free to call an instance from multiple threads, but only one call simultaneously. A single thread may interleave calls to multiple instances, so implementations using ThreadLocals must be careful to avoid leaking inappropriate state (e.g., clearing the ThreadLocal before returning).

[ST-DDT]

The StreamObserver already has a similar comment, so I skipped it:
https://github.com/ST-DDT/grpc-java/blob/javadoc/interceptor-concurrency/stub/src/main/java/io/grpc/stub/StreamObserver.java#L28-L34

[ejona86]

@ST-DDT, thank you! It is very good to be more clear here.