Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add implementation note regarding server interceptors and thread locals #7482

Merged
merged 2 commits into from Oct 13, 2020
Merged

Add implementation note regarding server interceptors and thread locals #7482

merged 2 commits into from Oct 13, 2020

Conversation

ST-DDT
Copy link
Contributor

@ST-DDT ST-DDT commented Oct 3, 2020

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

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.

@ericgribkoff ericgribkoff added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 6, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 6, 2020
@ejona86
Copy link
Member

ejona86 commented Oct 6, 2020

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).

@ejona86 ejona86 requested a review from voidzcy October 13, 2020 14:39
@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2020

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 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 13, 2020
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Oct 13, 2020
@ejona86 ejona86 merged commit b735505 into grpc:master Oct 13, 2020
@ejona86
Copy link
Member

ejona86 commented Oct 13, 2020

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

@ST-DDT ST-DDT deleted the javadoc/interceptor-concurrency branch October 13, 2020 20:43
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants