Visitar URL original
api: Clarify expectations regarding ServerCall#close by ST-DDT · Pull Request #7580 · grpc/grpc-java · GitHub
Skip to content

api: Clarify expectations regarding ServerCall#close - #7580

Merged
ejona86 merged 3 commits into
grpc:masterfrom
ST-DDT:docs/server-call-close-clarification
Nov 2, 2020
Merged

api: Clarify expectations regarding ServerCall#close#7580
ejona86 merged 3 commits into
grpc:masterfrom
ST-DDT:docs/server-call-close-clarification

Conversation

@ST-DDT

@ST-DDT ST-DDT commented Oct 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
ejona86
ejona86 reviewed Oct 30, 2020
View reviewed changes
Comment thread api/src/main/java/io/grpc/ServerCall.java Outdated
* <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

@ejona86 ejona86 Oct 30, 2020

Copy link
Copy Markdown
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

ejona86 commented Oct 30, 2020

Copy link
Copy Markdown
Member

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

ST-DDT commented Oct 30, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 reviewed Oct 30, 2020
View reviewed changes
Comment thread api/src/main/java/io/grpc/ServerCall.java Resolved
@ST-DDT ST-DDT mentioned this pull request Nov 1, 2020
Merged
@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
@ejona86
ejona86 requested a review from dapengzhang0 November 2, 2020 17:45
dapengzhang0
dapengzhang0 approved these changes Nov 2, 2020
View reviewed changes

@dapengzhang0 dapengzhang0 left a comment

Copy link
Copy Markdown
Contributor

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

ejona86 commented Nov 2, 2020

Copy link
Copy Markdown
Member

@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
@ST-DDT @dfawley
api: Clarify expectations regarding ServerCall#close (grpc#7580)
1b601fb
@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.

Reviewers

@ejona86 ejona86 ejona86 approved these changes
+1 more reviewer
@dapengzhang0 dapengzhang0 dapengzhang0 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

4 participants

@ST-DDT @ejona86 @dapengzhang0 @grpc-kokoro