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

Chunked responses #71

Merged
merged 5 commits into from
Dec 21, 2012
Merged

Chunked responses #71

merged 5 commits into from
Dec 21, 2012

Conversation

chetan
Copy link
Contributor

@chetan chetan commented Dec 19, 2012

I added support for chunked responses where a block is used to read the response body one chunk at a time instead of simply returning it as a string. In my case, I need to handle file downloads and the like. So far I've implemented it for Curb, HttpClient and Net::HTTP. Haven't looked into the EM libraries yet, but should be straightforward to add those as well.

@rubiii
Copy link
Contributor

rubiii commented Dec 20, 2012

looks pretty good to me so far. are you planning to implement this for all adapters (if they support this)?

@chetan
Copy link
Contributor Author

chetan commented Dec 20, 2012

I'm mainly interested in Curb & HttpClient myself, so I did this first. Net::HTTP I threw in just for kicks. I can definitely look into the EM adapter, I don't it should be too difficult, assuming it is supported. I can also take a look at the 1.8 and ree builds that failed. I'm guessing Net::HTTP changed a bit in 1.9 or something.

@rubiii
Copy link
Contributor

rubiii commented Dec 20, 2012

that would be awesome 😄

@chetan
Copy link
Contributor Author

chetan commented Dec 20, 2012

I managed to fix the ruby1.8 issue, but EM is proving more difficult. em-http works fine with streaming responses, but not when using synchrony. It seems that the way synchrony rewrites the synchronous request methods, it waits for the request to complete before resuming operation, so there's no chance of attaching the callback. We could rewrite/patch the synchrony methods here:

https://github.com/igrigorik/em-synchrony/blob/master/lib/em-synchrony/em-http.rb

but I'm not sure if that would be ideal. I'm also not very familiar with EM in general.

@rubiii
Copy link
Contributor

rubiii commented Dec 20, 2012

i see. i think it's ok to leave this feature out for em until someone needs it.
could you raise a NotSupportedError like we're already doing it in similar cases for em?

@rogerleite
Copy link
Member

This is a great feature!
Could you please rebase your branch. I have merged another pull right now.

@chetan
Copy link
Contributor Author

chetan commented Dec 21, 2012

@rogerleite done

rogerleite added a commit that referenced this pull request Dec 21, 2012
@rogerleite rogerleite merged commit 03876f7 into savonrb:master Dec 21, 2012
@rogerleite
Copy link
Member

@chetan I'm adding Chunked Responses to httpirb.com.
Look this commit savonrb/httpirb.com@99462c9
A better example would be great, you can help me?

@chetan
Copy link
Contributor Author

chetan commented Dec 21, 2012

@rogerleite sure, in my case, I do something like this:

File.open("/tmp/google_index.html", "w") do |io|
  req = HTTPI::Request.new("http://www.google.com/")
  req.on_body { |data| io << data; data.length }
  HTTPI.get(req)
end

Which reminds me, I forgot that curb requires you to return the length of the data read from the block. It allows you to abort the connection by returning a smaller value:

http://rdoc.info/github/taf2/curb/Curl/Easy:on_body

I guess this could be abstracted out in the adapter so that the user's block is called inside a custom block, or just leave it up to the user.

if @request.on_body then
  client.on_body do |data|
    @request.on_body.call(data)
    data.length
  end
end

What do you think?

@rogerleite
Copy link
Member

I'm in favor to be abstracted out in the adapter.

On RFC 2616, Chunked Transfer Coding section 3.6.1, the only statement about this is "The chunked encoding is ended by any chunk whose size is zero, followed by the trailer, which is terminated by an empty line.". RFC don't define a standard way to abort reading chunk response.

I'm gonna change curb adapter to abstract data length like @chetan suggested. Ok @rubiii ?

@rubiii
Copy link
Contributor

rubiii commented Dec 26, 2012

@rogerleite sounds reasonable

@rubiii rubiii mentioned this pull request Jul 20, 2013
27 tasks
@rubiii
Copy link
Contributor

rubiii commented Jul 22, 2013

released with version 2.1.0.

@rubiii
Copy link
Contributor

rubiii commented Jul 22, 2013

@rogerleite just noticed that your documentation commit is missing from httpirb.com
and i could be wrong, but i think the curb-specific content-length-abstraction didn't make it into the release.
please correct me if i'm wrong.

@rogerleite
Copy link
Member

@rubiii you're right. My commit was on edge branch savonrb/httpirb.com@99462c9.
I'll open a pull request on httpirb.com with the changes.

About the "curb-specific content-length-abstraction", i don't remember probably i forgot.
I can do that too.

@rogerleite
Copy link
Member

@rubiii this change need changelog? I owe anything else?

@rubiii
Copy link
Contributor

rubiii commented Jul 25, 2013

i think that's it. thank you @rogerleite!
i deployed the documentation, so everything should be up to date now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants