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

stub: add ServerCallStreamObserver.setOnCloseHandler(...) (#5895) #8452

Merged
merged 11 commits into from Sep 21, 2021

Conversation

morgwai
Copy link
Contributor

@morgwai morgwai commented Aug 26, 2021

This allows for user code to be notified when the messages are actually put on the wire and the stream is closed.

The name onSuccessHandler was chosen over onCompleteHandler to avoid confusion with StreamObserver.onCompleted(), but it is up to discussion of course.

This allows for user code to be notified when the messages are actually
put on the wire and the stream is closed.
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Looks pretty close to me.

"Success" seems pretty fair, since we can't use "complete." Only other option that comes immediately to mind is "done." I've added this to the agenda of our API review meeting this Thursday. (API review isn't a blocker for experimental APIs, but an initial review helps avoid annoying name changes later.)

The handler is called also after onError(...), so onSuccess sounds
confusing in this situation.
@morgwai morgwai changed the title stub: add ServerCallStreamObserver.setOnSuccessHandler(...) (#5895) stub: add ServerCallStreamObserver.setOnFinishHandler(...) (#5895) Sep 1, 2021
@morgwai morgwai requested a review from ejona86 September 1, 2021 16:07
@ejona86
Copy link
Member

ejona86 commented Sep 1, 2021

Finish does look better than Success. Thanks for the name brainstorming. We'll choose a name in the API meeting.

@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 Sep 2, 2021
@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 Sep 2, 2021
@morgwai
Copy link
Contributor Author

morgwai commented Sep 2, 2021

@ejona86
I've cleaned and rebuilt everything again, but cannot reproduce the error that occurred in the checks. Furthermore, looking at the output it does not seem to be related to my changes:

Task :grpc-netty:test

io.grpc.netty.AdvancedTlsTest > onFileReloadingKeyManagerTrustManagerTest FAILED
java.lang.AssertionError: Find error: UNAVAILABLE: io exception
Channel Pipeline: [SslHandler#0, ProtocolNegotiators$ClientTlsHandler#0, WriteBufferingAndExceptionHandler#0, DefaultChannelPipeline$TailContext#0]
at org.junit.Assert.fail(Assert.java:88)
at io.grpc.netty.AdvancedTlsTest.onFileReloadingKeyManagerTrustManagerTest(AdvancedTlsTest.java:385)

any hints how to deal with it? ;-)

@ejona86
Copy link
Member

ejona86 commented Sep 2, 2021

@morgwai, don't worry about it. #8454

@morgwai morgwai changed the title stub: add ServerCallStreamObserver.setOnFinishHandler(...) (#5895) stub: add ServerCallStreamObserver.setOnCloseHandler(...) (#5895) Sep 3, 2021
@morgwai
Copy link
Contributor Author

morgwai commented Sep 3, 2021

if #8476 is indeed a bug (as opposed to documentation omission, that I suspect), then onCloseHandler may be useful also for streaming clients, so that they do not exit before trailing metadata is transffered. In such case setOnCloseHandler should probably be moved to CallStreamObserver.

@ejona86
Copy link
Member

ejona86 commented Sep 3, 2021

then onCloseHandler may be useful also for streaming clients

The onCloseHandler is powered by ServerCall.Listener.onComplete(). On client-side, the equivalent would be ClientCall.Listener.onClose(). However, ClientCall.Listener.onClose() already calls StreamObserver.onCompleted() or StreamObserver.onError(), depending on the status code. So on client-side the equivalent callback(s) already exist.

@morgwai morgwai requested a review from ejona86 September 4, 2021 08:56
@morgwai
Copy link
Contributor Author

morgwai commented Sep 7, 2021

@ejona86 I've applied most of your comments. Please have a look at the one I was not sure about if you like the way it is now.

@morgwai
Copy link
Contributor Author

morgwai commented Sep 16, 2021

@ejona86 friendly ping :)
Please let me know if the current doc is ok or if you want me to change it somehow. Thanks!

@ejona86
Copy link
Member

ejona86 commented Sep 16, 2021

Oh, thanks for the ping. Yes, this had been lost. I'll try and take a look tomorrow.

@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 Sep 17, 2021
@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 Sep 17, 2021
@ejona86 ejona86 merged commit a6abb1b into grpc:master Sep 21, 2021
@ejona86
Copy link
Member

ejona86 commented Sep 21, 2021

Thank you, @morgwai! Thanks for putting up with the process.

@morgwai
Copy link
Contributor Author

morgwai commented Sep 22, 2021

@ejona86 my pleasure :) thanks for the review!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 21, 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

4 participants