Skip to content

Commit

Permalink
grpc: close stream with data frame (#500)
Browse files Browse the repository at this point in the history
Description: the gRPC protocol requires closing the client stream in a DATA frame. This PR fixes the GRPCStreamEmitter for iOS.
Risk Level: med - protocol level changes.
Testing: local.

Signed-off-by: Jose Nino <[email protected]>
  • Loading branch information
junr03 authored Oct 15, 2019
1 parent ef58f94 commit cf8f2c8
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 22 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@ class EnvoyStreamEmitter(
/**
* For ending an associated stream and sending trailers.
*
* @param trailers to send with ending a stream. If no trailers are needed, empty map will be the default.
* @param trailers to send with ending a stream. If null, stream will be closed with an empty data frame.
* @throws IllegalStateException when the stream is not active.
* @throws EnvoyException when there is an exception ending the stream or sending trailers.
*/
override fun close(trailers: Map<String, List<String>>) {
stream.sendTrailers(trailers)
override fun close(trailers: Map<String, List<String>>?) {
trailers?.let {
stream.sendTrailers(it)
return
}
stream.sendData(ByteBuffer.allocate(0), true)
}

/**
Expand Down
4 changes: 2 additions & 2 deletions library/kotlin/src/io/envoyproxy/envoymobile/StreamEmitter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ interface StreamEmitter : CancelableStream {
/**
* For ending an associated stream and sending trailers.
*
* @param trailers to send with ending a stream. If no trailers are needed, empty map will be the default.
* @param trailers to send with ending a stream. If null, stream will be closed with an empty data frame.
* @throws IllegalStateException when the stream is not active.
* @throws EnvoyException when there is an exception ending the stream or sending trailers.
*/
@Throws(EnvoyException::class)
fun close(trailers: Map<String, List<String>> = emptyMap())
fun close(trailers: Map<String, List<String>>?)
}
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ class EnvoyClientTest {
}

@Test
fun `closing stream sends empty trailers to the underlying stream`() {
fun `closing stream sends empty data to the underlying stream`() {
`when`(engine.startStream(any())).thenReturn(stream)
val envoy = Envoy(engine, config)

Expand All @@ -97,9 +97,9 @@ class EnvoyClientTest {
.build(),
ResponseHandler(Executor {}))

emitter.close()
emitter.close(null)

verify(stream).sendTrailers(emptyMap())
verify(stream).sendData(ByteBuffer.allocate(0), true)
}

@Test
Expand Down
8 changes: 6 additions & 2 deletions library/swift/src/EnvoyStreamEmitter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,12 @@ extension EnvoyStreamEmitter: StreamEmitter {
return self
}

func close(trailers: [String: [String]]) {
self.stream.sendTrailers(trailers)
func close(trailers: [String: [String]]?) {
if let trailers = trailers {
self.stream.sendTrailers(trailers)
} else {
self.stream.send(Data(), close: true)
}
}

func cancel() {
Expand Down
5 changes: 4 additions & 1 deletion library/swift/src/GRPCStreamEmitter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public final class GRPCStreamEmitter: NSObject {

/// Close this connection.
public func close() {
self.underlyingEmitter.close()
// The gRPC protocol requires the client stream to close with a DATA frame.
// More information here:
// https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#requests
self.underlyingEmitter.close(trailers: nil)
}
}
14 changes: 4 additions & 10 deletions library/swift/src/StreamEmitter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,9 @@ public protocol StreamEmitter: CancelableStream {
@discardableResult
func sendMetadata(_ metadata: [String: [String]]) -> StreamEmitter

/// End the stream after sending any provided trailers.
/// End the stream.
///
/// - parameter trailers: Trailers to send over the stream.
func close(trailers: [String: [String]])
}

extension StreamEmitter {
/// Convenience function for ending the stream without sending any trailers.
public func close() {
self.close(trailers: [:])
}
/// - parameter trailers: Trailers with which to close the stream.
// If nil, stream will be closed with an empty data frame.
func close(trailers: [String: [String]]?)
}
2 changes: 1 addition & 1 deletion library/swift/test/GRPCStreamEmitterTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ private final class MockEmitter: StreamEmitter {
return self
}

func close(trailers: [String: [String]]) {}
func close(trailers: [String: [String]]?) {}
func cancel() {}
}

Expand Down

0 comments on commit cf8f2c8

Please sign in to comment.