Skip to content

Commit

Permalink
Make sure we don't write additional DATA frames when flushing once re…
Browse files Browse the repository at this point in the history
…quest is finished (#19)

## Motivation
When a client sends a request, to signal its end to the server, it has
to send an empty DATA frame with END_STREAM set.
In the current implementation, the state machine correctly signals the
handler that there are no more messages to be encoded, and so the client
proceeds to write the empty DATA frame with EOS set when flushing.
However, if subsequent flushes are triggered down the pipeline, the
client will keep writing these empty DATA frames, which is a bug.

## Modifications
Make sure that we only write the empty DATA frame once. If we've already
done it, the client can ignore subsequent flushes, as it is done.
  • Loading branch information
gjcairo authored Dec 4, 2024
1 parent 5152e8f commit 5cd9f91
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ final class GRPCClientStreamHandler: ChannelDuplexHandler {

private var isReading = false
private var flushPending = false
private var requestFinished = false

init(
methodDescriptor: MethodDescriptor,
Expand Down Expand Up @@ -263,6 +264,12 @@ extension GRPCClientStreamHandler {
)

case .noMoreMessages:
// If we're done writing (i.e. we have no more messages returned from state
// machine) then return.
if self.requestFinished { return }

self.requestFinished = true

// Write an empty data frame with the EOS flag set, to signal the RPC
// request is now finished.
context.write(
Expand All @@ -276,7 +283,6 @@ extension GRPCClientStreamHandler {
),
promise: nil
)

context.flush()
break loop

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -529,14 +529,16 @@ final class GRPCClientStreamHandlerTests: XCTestCase {
// Half-close the outbound end: this would be triggered by finishing the client's writer.
XCTAssertNoThrow(channel.close(mode: .output, promise: nil))

// Flush to make sure the EOS is written.
channel.flush()

// Make sure the EOS frame was sent
let emptyEOSFrame = try channel.assertReadDataOutbound()
XCTAssertEqual(emptyEOSFrame.data, .byteBuffer(.init()))
XCTAssertTrue(emptyEOSFrame.endStream)

// Make sure that, if we flush again, we're not writing anything else down
// the stream. We should have closed at this point.
channel.flush()
XCTAssertNil(try channel.readOutbound(as: HTTP2Frame.FramePayload.self))

// Make sure we cannot write anymore because client's closed.
XCTAssertThrowsError(
ofType: RPCError.self,
Expand Down

0 comments on commit 5cd9f91

Please sign in to comment.