Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trailers sent on idle streams deserve to be sent too. #25

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Nov 16, 2018

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.
Resolves #24.

@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 16, 2018

Note: this issue blocks grpc/grpc-swift#281.

Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit... otherwise LGTM

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

XCTAssertNoThrow ?

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy but I agree with @normanmaurer that we should sprinkle some XCTAssertNoThrows everywhere :)

@MrMage
Copy link
Contributor

MrMage commented Nov 16, 2018

I can confirm that this fixes the issue I had been encountering. Thank you so much, Corey!

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.
@Lukasa
Copy link
Contributor Author

Lukasa commented Nov 16, 2018

I've updated to use XCTAssertNoThrow.

@weissi
Copy link
Member

weissi commented Nov 18, 2018

Thanks, still happy 👌

Copy link
Member

@normanmaurer normanmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it...

@Lukasa Lukasa merged commit c095189 into apple:master Nov 21, 2018
@Lukasa Lukasa deleted the cb-issue-24 branch November 21, 2018 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants