Add support for accessing headers and trailers to streaming calls #171
Conversation
…red, which completes when the headers or trailers are received
jhump
force-pushed
the
jh/headers-trailers
branch
from
70ec37c
to
5e5a061
Compare
| } | ||
| } | ||
|
|
||
| // TODO: remove this method as it is redundant with receive closed |
There was a problem hiding this comment.
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.
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.
| @@ -76,11 +76,14 @@ class Stream( | |||
| fun receiveClose() { | |||
| if (!isReceiveClosed.getAndSet(true)) { | |||
| onReceiveClose() | |||
| // closing receive side implicitly closes send side, too | |||
| isSendClosed.set(true) | |||
There was a problem hiding this comment.
Should we call sendClose() instead here so the onSendClose() callback is always invoked? Do we want to invoke this in a try / finally to ensure it will always be run even if an exception occurs in onReceiveClose?
There was a problem hiding this comment.
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 call sendClose() 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 leave isSendClosed set to false when isReceiveClosed is set to true. So I guess I've talked myself into needing a finally here.
There was a problem hiding this comment.
Updated to use try/finally and added a comment as to why this isn't calling sendClose().
… test code, so not safe to assert they are already complete
|
@pkwarren, I added some assertions to existing test cases related to headers with streaming operations. PTAL |
This is done via a
CompletableDeferredwhich is completed when the headers/trailers are received.