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

send partial bodies #55

Merged
merged 4 commits into from
Apr 7, 2016
Merged

Conversation

stillinbeta
Copy link
Contributor

adds http2_connection:send_body/4, which can take an arg
{send_rst_stream, false}. This lets handlers call send_body
multiple times instead of needing to buffer output

@joedevivo
Copy link
Owner

The name of this option should be 'end_stream', 'rst_stream' is for error
cases

On Thursday, March 31, 2016, Liz notifications@github.com wrote:

adds http2_connection:send_body/4, which can take an arg
{send_rst_stream, false}. This lets handlers call send_body

multiple times instead of needing to buffer output

You can view, comment on, or merge this pull request online at:

#55
Commit Summary

  • Tests and skeleton for new send body
  • Support sending body in multiple segments

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#55

@stillinbeta
Copy link
Contributor Author

no idea why that test's failing, and I can't seem to repro it locally. maybe a flaky test?

@joedevivo
Copy link
Owner

It's failing consistently in travis, which makes me think it's not a flaky test. It does work for me locally, so I'm not really sure what's going on, but I'll investigate.

@joedevivo
Copy link
Owner

Looks like I finally got one to pass, but I need to see some consistency.

@joedevivo
Copy link
Owner

Four in a row passed, so I'll look at the code now :D


ExpectedResponseBody = <<"BodyPart1\nBodyPart2">>,

{ok, {ResponseHeaders, ResponseBody}} = http2_client:sync_request(Client, RequestHeaders, <<>>),
Copy link
Owner

Choose a reason for hiding this comment

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

There's a better way to test this using http2c. It will allow you to inspect the response as a series of frames. You can then assert that the response is made of two data frames, and that they're broken up correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's a level of abstraction lower than we need to test. as a user I don't care how you send my data, just that you do as fast as possible.

Copy link
Owner

Choose a reason for hiding this comment

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

It's not really doing that though. For better or worse, this test would pass if the server held back all the response body until it was ready to send it all.

It might mean that we have to add some kind of streaming functionality to the http2_client, so that we can see the response coming in pieces. We might have to add some kind of timer delay to the double_body_handler to take advantage of that.

@stillinbeta
Copy link
Contributor Author

Just travis things ¯_(ツ)_/¯

{NewSWS, NewS} =
s_send_what_we_can(Conn#connection.send_window_size,
Conn#connection.send_settings#settings.max_frame_size,
Stream#stream{
queued_data=Body
queued_data=Body,
Copy link
Owner

Choose a reason for hiding this comment

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

Need to change this line to something along the lines of

queued_data=<<Stream#stream.queued_data,Body>>,

but really it needs to be safer than that, something like

NewBody = case is_binary(Stream#stream.queued_data) of
    true ->
        <<Stream#stream.queued_data,Body>>;
    false ->
        Body
end,
{NewSWS, NewS} =
   s_send_what_we_can(Conn#connection.send_window_size,
                      Conn#connection.send_settings#settings.max_frame_size,
                      Stream#stream{
                          queued_data=NewBody,

That and a supporting test and this PR should be ready to merge

@joedevivo
Copy link
Owner

A+! Would merge again

@joedevivo joedevivo merged commit 4b167ec into joedevivo:master Apr 7, 2016
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.

2 participants