Skip to content

Commit

Permalink
Trailers sent on idle streams deserve to be sent too. (#25)
Browse files Browse the repository at this point in the history
Motivation:

Due to nghttp2's weird data provider model, all trailers go through a
similar send path as data frames. This includes the fact that the data
sender can go "idle". If the data sender is idle, and the user sends
trailers, we don't bother to tell nghttp2 to ask us for more data,
so the trailers sit there forever, feeling sad.

Modifications:

- Correctly restart the data provider when sending data.

Result:

Trailers will be emitted when the stream is idle.
  • Loading branch information
Lukasa authored Nov 21, 2018
1 parent d24bb5d commit c095189
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 1 deletion.
8 changes: 8 additions & 0 deletions Sources/NIOHTTP2/NGHTTP2Session.swift
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,14 @@ class NGHTTP2Session {
// TODO(cory): Error handling.
precondition(isEndStream)
streamData.dataProvider.bufferEOF(trailers: headers)

if case .pending = streamData.dataProvider.state {
// The data provider is currently in the pending state, we need to tell nghttp2 it's active again so
// the trailers get emitted.
let rc = nghttp2_session_resume_data(self.session, frame.streamID.networkStreamID!)
precondition(rc == 0)
streamData.dataProvider.didResume()
}
return
}

Expand Down
1 change: 1 addition & 0 deletions Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ extension SimpleClientServerTests {
("testFailingUnflushedWritesForGoaway", testFailingUnflushedWritesForGoaway),
("testFailingFlushedWritesForGoaway", testFailingFlushedWritesForGoaway),
("testSendingTrailers", testSendingTrailers),
("testSendingTrailersWhenDataProviderIsIdle", testSendingTrailersWhenDataProviderIsIdle),
("test1XXResponseHeaderFields", test1XXResponseHeaderFields),
("testPriorityFramesAreNotEmitted", testPriorityFramesAreNotEmitted),
("testStreamClosedWithNoError", testStreamClosedWithNoError),
Expand Down
35 changes: 35 additions & 0 deletions Tests/NIOHTTP2Tests/SimpleClientServerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,41 @@ class SimpleClientServerTests: XCTestCase {
XCTAssertNoThrow(try self.serverChannel.finish())
}

func testSendingTrailersWhenDataProviderIsIdle() throws {
// Begin by getting the connection up.
try self.basicHTTP2Connection()

// We're now going to try to send a request from the client to the server. This request will be one headers frame, one
// data frame, and then another headers frame for trailers. The headers frame for trailers will be sent late, to
// reproduce https://github.com/apple/swift-nio-http2/issues/24.
let headers = HTTPHeaders([(":path", "/"), (":method", "POST"), (":scheme", "https"), (":authority", "localhost")])
let trailers = HTTPHeaders([("x-trailers-field", "true")])
var requestBody = self.clientChannel.allocator.buffer(capacity: 128)
requestBody.write(staticString: "A simple HTTP/2 request.")

let clientStreamID = HTTP2StreamID()
let reqFrame = HTTP2Frame(streamID: clientStreamID, payload: .headers(headers))
let reqBodyFrame = HTTP2Frame(streamID: clientStreamID, payload: .data(.byteBuffer(requestBody)))
var trailerFrame = HTTP2Frame(streamID: clientStreamID, payload: .headers(trailers))
trailerFrame.flags.insert(.endStream)

let serverStreamID = try self.assertFramesRoundTrip(frames: [reqFrame, reqBodyFrame], sender: self.clientChannel, receiver: self.serverChannel).first!.streamID

// Now we can send the next trailers.
XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [trailerFrame], sender: self.clientChannel, receiver: self.serverChannel))

// Let's send a quick response back. This response should also contain trailers.
let responseHeaders = HTTPHeaders([(":status", "200"), ("content-length", "0")])
let respFrame = HTTP2Frame(streamID: serverStreamID, payload: .headers(responseHeaders))
var respTrailersFrame = HTTP2Frame(streamID: serverStreamID, payload: .headers(trailers))
respTrailersFrame.flags.insert(.endStream)
XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [respFrame], sender: self.serverChannel, receiver: self.clientChannel))
XCTAssertNoThrow(try self.assertFramesRoundTrip(frames: [respTrailersFrame], sender: self.serverChannel, receiver: self.clientChannel))

XCTAssertNoThrow(try self.clientChannel.finish())
XCTAssertNoThrow(try self.serverChannel.finish())
}

func test1XXResponseHeaderFields() throws {
// Begin by getting the connection up.
try self.basicHTTP2Connection()
Expand Down
2 changes: 1 addition & 1 deletion Tests/NIOHTTP2Tests/TestUtilities.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ extension XCTestCase {
var receivedFrames = [HTTP2Frame]()

for frame in frames {
let receivedFrame = try receiver.assertReceivedFrame()
let receivedFrame = try receiver.assertReceivedFrame(file: file, line: line)
receivedFrame.assertFrameMatches(this: frame, file: file, line: line)
receivedFrames.append(receivedFrame)
}
Expand Down

0 comments on commit c095189

Please sign in to comment.