-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
This allows for user code to be notified when the messages are actually put on the wire and the stream is closed.
There was a problem hiding this 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.
Finish does look better than Success. Thanks for the name brainstorming. We'll choose a name in the API meeting. |
Fix some formatting and references.
@ejona86
any hints how to deal with it? ;-) |
if #8476 is indeed a bug (as opposed to documentation omission, that I suspect), then |
The onCloseHandler is powered by |
@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. |
@ejona86 friendly ping :) |
Oh, thanks for the ping. Yes, this had been lost. I'll try and take a look tomorrow. |
Thank you, @morgwai! Thanks for putting up with the process. |
@ejona86 my pleasure :) thanks for the review! |
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 overonCompleteHandler
to avoid confusion withStreamObserver.onCompleted()
, but it is up to discussion of course.