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

HTTP::ResponseBody #39

Merged
merged 4 commits into from
Jan 17, 2014
Merged

HTTP::ResponseBody #39

merged 4 commits into from
Jan 17, 2014

Conversation

tarcieri
Copy link
Member

This encapsulates HTTP response bodies in their own object which can be streamed incrementally. Seems useful for the Twitter gem there, @sferik ;)

It includes a number of other changes for features I don't want to support any more, but can arguably be added back in:

  • Callback system: No longer needed when we control the request lifecycle. We can just call methods at the appropriate times. Death to callbacks!
  • Automagical request parsing: We need to figure out a better API for this. Perhaps we could add an HTTP::ResponseBody#parse method to take its place. Seems simple enough, and could be completely automatic based on the response's Content-Type

Inspiration

I would like to get the streaming support in the Http Gem on par with what just shipped in Reel. Reel now has support for HTTP Pipelining as well as streaming both request and response bodies, and I think The Http Gem should as well.

I think The HTTP Gem can be restructured to work more like Reel, and this branch contains some of that work in progress. Here are the relevant classes in the HTTP Gem, how they map onto the corresponding classes in Reel:

  • HTTP::Client => Reel::Connection: Arguably there should be a HTTP::Connection object too. The HTTP Gem does a bad job of handling connection pooling and keep-alive :( In Reel this class manages the connection lifecycle. The HTTP Gem presently only supports a single request per connection. This class feeds data to the parser so it can be consumed by the other classes
  • HTTP::ResponseParser => Reel::RequestParser: Parses the incoming data (a response in the case of the HTTP Gem, or a request in the case of Reel). The parser hands the data off to the active request/response object.
  • HTTP::Response => Reel::Request: Serves as the interface between Reel::Connection objects and Reel::RequestBodies. This is important with HTTP pipelining, as we may be serving a different response fron the one we're currently reading. HTTP::Response would serve as the indirection mechanism for the response we're currently reading
  • HTTP::ResponseBody => Reel::RequestBody: access to the response body. Can be consumed in one fell swoop with #to_s, but also provides a #readpartial API for reading the body in chunks, and is Enumerable so it can be consumed with those methods too.

I feel like there's a lot of logical overlap between Reel and The HTTP Gem, even though one's a client and the other's a server ;) Perhaps there's a way to extract it into a single gem to manage the HTTP lifecycle (perhaps this gem!)

@indirect
Copy link

DO WANT.

@tarcieri
Copy link
Member Author

cc @sferik @indirect

Did some more work on this tonight and the tests now pass! Well, mostly...

Redirects are broken. This change is a bit hack-and-slash as the old code was rather wonky. Feedback would be appreciated, and if anyone can fix the redirect handling that'd be awesome as well.

Since the next release will be a breaking API change anyway it might be a good time to get this in. This is probably the last major change I want to make to the API prior to a 1.0 release, other than removing the proxy API (which is broken)

tarcieri and others added 4 commits January 16, 2014 01:02
This factors all of the concerns of processing the response body into a separate
HTTP::ResponseBody object. This is a *BREAKING* API change, but hopefully one
that results in both a better API and a cleaner, more maintainable codebase.

It also deletes the callback system, which is 1) unnecessary 2) getting in the
way of the refactor.

Redirects are presently broken and marked as pending.
Fixes issues in handling redirects arising from the HTTP::ResponseBody refactor
The tests should pass now, in theory!
@indirect
Copy link

I like it! I have basically two use cases: messing around in the console (this looks great) and Bundler making persistent, pipelined connections over HTTPS. Right now we use net-http-persistent and net-http-pipeline for that, but I will happily switch for the nicer API. :P

@tarcieri
Copy link
Member Author

This paves the way for pipelining (we're using a similar setup on the server side for Reel pipelining) but doesn't implement it yet.

The main goal, as you've noted, is a nicer API.

@tarcieri
Copy link
Member Author

Tested this last night with the twitter gem and this change did not affect it (probably since it isn't touching this codepath).

Unless there's any objections I'd like to merge this.

@sferik
Copy link
Contributor

sferik commented Jan 17, 2014

@tarcieri When I suggested hooking up the twitter gem, I didn’t mean adding it to the Gemfile and make sure nothing breaks. I meant swapping in the new response object and seeing how it works in the real world. Could you give that a try before merging? I’m happy to help with this.

@tarcieri
Copy link
Member Author

@sferik originally requests returned a SimpleDelegator to a string containing the response body, and you could call #response on it to get the response object. This change returns response objects directly. You can still get to the body as a string by calling #to_s (this was all unless you said #response(:object) to ask for the body as an object explicitly)

My gut feeling is the old API was clunky and this API is a lot cleaner. It should also make it much easier to stream data from e.g. the Twitter API, because you can use HTTP::ResponseBody#readpartial to stream the data as if the body were an IO object. This mirrors the API in Reel.

I can help with hooking this up in the twitter gem, but why do you want to do that before merging this change?

@sferik
Copy link
Contributor

sferik commented Jan 17, 2014

@tarcieri If you’re happy with the API, then let’s merge it. I just thought that using it in the real-world would give you some more insight into how the API is to use.

sferik added a commit that referenced this pull request Jan 17, 2014
@sferik sferik merged commit f32153d into master Jan 17, 2014
@tarcieri
Copy link
Member Author

:shipit:

@tarcieri tarcieri deleted the response-bodies branch January 17, 2014 21:32
@tarcieri
Copy link
Member Author

BTW, I do plan a few follow-up commits after this to clean up the code and improve test coverage

@ixti ixti mentioned this pull request Jan 19, 2014
@dgollahon dgollahon mentioned this pull request Feb 9, 2019
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.

None yet

3 participants