platform: close streams explicitly with data or trailers #798
Conversation
Replaces the `close(trailers: Map<String, List<String>>?)` function with two new functions (one for closing with data, and one for closing with trailers). This removes the ambiguity of how a stream will be closed by making the consumer specify it explicitly. The change is being done as a precursor for integrating L7 Swift/Kotlin filters, which will have interfaces in which trailers are not nullable. Signed-off-by: Michael Rebello <me@michaelrebello.com>
| return | ||
| } | ||
| stream.sendData(ByteBuffer.allocate(0), true) | ||
| override fun close(trailers: Map<String, List<String>>) { |
There was a problem hiding this comment.
They're specified in the superclass, so I figured it was better to have them in one place rather than copy-pasted to every implementation. WDYT?
There was a problem hiding this comment.
Could we also delete the other comments? If it's a lot to change, we can both do it in another PR
There was a problem hiding this comment.
I'll do it in a follow up PR yep, waiting on this to merge to avoid conflicts
There was a problem hiding this comment.
| stream.sendTrailers(trailers) | ||
| } | ||
|
|
||
| override fun close(byteBuffer: ByteBuffer) { |
There was a problem hiding this comment.
What was the rationale for making this required (instead of having something like fun close(byteBuffer: ByteBuffer = ByteBuffer.allocate(0)
There was a problem hiding this comment.
It'd be ambiguous if we did that for trailers as well, right? Consumers calling .close() would no longer be able to differentiate between data and trailers
There was a problem hiding this comment.
Ah so the change here is to introduce the concept of closing with data versus closing with trailers
There was a problem hiding this comment.
Related to closing the stream. Right now we don't have a convenience API for a headers only request. For instance a send method that does not return a StreamEmitter because you send with as an only headers request. Thoughts on fixing that here vs in a different PR?
Yep I'll go ahead and update some of that in a follow up PR |
Replaces the
close(trailers: Map<String, List<String>>?)function with two new functions (one for closing with data, and one for closing with trailers).This removes the ambiguity of how a stream will be closed by making the consumer specify explicitly whether they'd like to close a stream using trailers or data.
The change is being done as a precursor for integrating L7 Swift/Kotlin filters, which will have interfaces in which trailers are not nullable.
Signed-off-by: Michael Rebello me@michaelrebello.com