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

Simplify HttpTransportOverHTTP2 #2420

Closed
sbordet opened this issue Apr 7, 2018 · 4 comments
Closed

Simplify HttpTransportOverHTTP2 #2420

sbordet opened this issue Apr 7, 2018 · 4 comments

Comments

@sbordet
Copy link
Contributor

sbordet commented Apr 7, 2018

HttpTransportOverHTTP2.send() method has the logic to write HTTP/2 frames and must deal with commit, lastContent, trailers and endStream.

The current implementation makes use of ternary expressions that, while compact, make difficult to follow the logic.

@sbordet sbordet closed this as completed in 06454f6 Apr 7, 2018
@gregw
Copy link
Contributor

gregw commented Apr 7, 2018

I think the if another issue in this class. On commit it send a header frame and then only sends the data frame once the callback is succeeded. But the headers frame may be interleaved with many other frames resulting in a delay between headers and content. Also 3 buffers must be used: headers frame, data frame prefix and data slice.

If a sendHeadersAndDataPsuedoFrame was available, then the flusher could use a single buffer for the headers frame and the data frame prefix, followed by the data slice. This would be better on the wire, use less buffers and have only a single call back cycle.

@gregw gregw reopened this Apr 7, 2018
@sbordet
Copy link
Contributor Author

sbordet commented Apr 7, 2018

@gregw that seems incredibly complicated.
It does not save on number of writes and only sends down 9 frame header bytes that will stay on the receiver parser state until the content comes.
Plus, I cannot send the 9 frame headers bytes in advance because they contain the frame length. If I send the content later, I may not be able to send the promised content because the flow control window changed.

@sbordet sbordet closed this as completed Apr 7, 2018
@gregw
Copy link
Contributor

gregw commented Apr 8, 2018

@sbordet, but you have the headers and the content at the same time in the send method. All you need is a psuedo frame type that contains the info for headers and data. When it generates it creates the headers frame, then the data frame prefix (into the same leased buffer) then it adds the sliced data buffer.

If send only has headers then only send headers. If send only has data then send only data. But if it had both then send both!

It will save a lot and the other end will be faster as well as the headers and data will not be split by writes nor interleaved with other frames.

It is a simplification of the code as you do not need nested callbacks.

Have a try at least.

@sbordet
Copy link
Contributor Author

sbordet commented Apr 8, 2018

Moved to #2423.

@sbordet sbordet closed this as completed Apr 8, 2018
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

No branches or pull requests

2 participants