-
Notifications
You must be signed in to change notification settings - Fork 17
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 support for accessing headers and trailers to streaming calls #171
Changes from 1 commit
5e5a061
756c3b0
371f1c0
7a77ef4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,7 +56,7 @@ class Stream( | |
private val isReceiveClosed = AtomicReference(false) | ||
|
||
fun send(buffer: Buffer): Result<Unit> { | ||
if (isClosed()) { | ||
if (isSendClosed()) { | ||
return Result.failure(IllegalStateException("cannot send. underlying stream is closed")) | ||
} | ||
return try { | ||
|
@@ -76,11 +76,14 @@ class Stream( | |
fun receiveClose() { | ||
if (!isReceiveClosed.getAndSet(true)) { | ||
onReceiveClose() | ||
// closing receive side implicitly closes send side, too | ||
isSendClosed.set(true) | ||
} | ||
} | ||
|
||
// TODO: remove this method as it is redundant with receive closed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems fine to remove the method if it isn't needed. I don't really see any consumers of this anywhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's referenced indirectly in all three of the stream interfaces and their impls. I figured I'd put that diff churn in a separate PR where I also hope to do some other improvement/overhauling of the stream interfaces. |
||
fun isClosed(): Boolean { | ||
return isSendClosed() && isReceiveClosed() | ||
return isReceiveClosed() | ||
} | ||
|
||
fun isSendClosed(): Boolean { | ||
|
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.
Should we call
sendClose()
instead here so theonSendClose()
callback is always invoked? Do we want to invoke this in atry / finally
to ensure it will always be run even if an exception occurs inonReceiveClose
?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.
We don't want
onSendClose()
to be invoked because that will try to half-close the stream (which should fail since the thing is already done at this point). And I don't think we want to callsendClose()
first (before doing the receive side) because that is just an extra, unnecessary network packet to send the half-close (technically an empty "end-of-stream" data frame) when the cancellation is all that is needed. As the comment says, closing the receive side of the stream implicitly closes the send -- so book-keeping is all that is needed here and no other action. I can add comments to this effect.Unclear about using try/finally. On the one hand, if
onReceiveClose
fails, then the stream may not be closed so that's an argument to not use finally. However, it doesn't really make much sense to leaveisSendClosed
set to false whenisReceiveClosed
is set to true. So I guess I've talked myself into needing a finally here.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.
Updated to use try/finally and added a comment as to why this isn't calling
sendClose()
.