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

api: Clarify expectations regarding ServerCall#close #7580

Merged
merged 3 commits into from Nov 2, 2020
Merged

api: Clarify expectations regarding ServerCall#close #7580

merged 3 commits into from Nov 2, 2020

Conversation

ST-DDT
Copy link
Contributor

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

Fixes #7571

@ST-DDT ST-DDT changed the title api: Carify behavior of ServerCall#close regarding client side cancellations api: Clarify behavior of ServerCall#close regarding client side cancellations Oct 30, 2020
@ST-DDT ST-DDT closed this Oct 30, 2020
@ST-DDT ST-DDT deleted the docs/server-call-close-clarification branch October 30, 2020 19:33
@@ -163,6 +163,11 @@ public boolean isReady() {
* <p>Since {@link Metadata} is not thread-safe, the caller must not access (read or write) {@code
* trailers} after this point.
*
* <p>Implementation note regarding ForwardingServerCalls: This method is called after the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd much rather not call out ForwardingServerCalls. And saying something like "this method won't be called" isn't right either as it is up to the application whether it is called or not.

I think we should have some text to emphasize that the close call is not the point where the RPC is complete. Instead, that is onComplete/onCancel. We can probably add that to the paragraph above. We can also say that close is not necessary to be called if the RPC is cancelled as the call is already dead.

@ejona86
Copy link
Member

ejona86 commented Oct 30, 2020

Oh, you closed. Was that an accident? This doesn't seem like something bad to improve.

@ST-DDT ST-DDT restored the docs/server-call-close-clarification branch October 30, 2020 20:50
@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 30, 2020

I closed this PR because the original purpose of the PR which is the second sentence is not true (see linked issue).

I'll reopen it with a changed description.

@ST-DDT ST-DDT reopened this Oct 30, 2020
@ST-DDT ST-DDT changed the title api: Clarify behavior of ServerCall#close regarding client side cancellations api: Clarify expectations regarding ServerCall#close Oct 30, 2020
@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 Nov 2, 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 Nov 2, 2020
Copy link
Member

@dapengzhang0 dapengzhang0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ejona86 ejona86 merged commit 566f16e into grpc:master Nov 2, 2020
@ejona86
Copy link
Member

ejona86 commented Nov 2, 2020

@ST-DDT, thank you! I appreciate these javadoc improvements.

@ST-DDT ST-DDT deleted the docs/server-call-close-clarification branch November 2, 2020 21:08
dfawley pushed a commit to dfawley/grpc-java that referenced this pull request Jan 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 5, 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.

ServerCall#close(Status, Metadata) never called for client-cancelled calls
4 participants