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

Rewind IO-alike request body after it was consumed. #468

Merged
merged 1 commit into from
Apr 24, 2018

Conversation

ixti
Copy link
Member

@ixti ixti commented Apr 24, 2018

Webmock triggers body.each twice upon inital request recording:

  1. to perform request
  2. to record request data into webmock request registry

But even without WebMock, generally Body#each must be replayable IMO.

Simple reproduction

This is a simple snippet that is doing something similar to what I was doing when I clashed this issue.

require "http"
require "webmock"
require "vcr"

VCR.configure do |config|
  config.hook_into :webmock
  config.cassette_library_dir = File.realpath("#{__dir__}/vcr")
end

VCR.use_cassette("httpbin", :match_requests_on => %i[method uri body]) do
  res = HTTP.post("https://httpbin.org/post", :form => { "a" => "z" })
  puts res.parse(:json)
end

@ixti ixti requested a review from janko April 24, 2018 14:48
@ixti
Copy link
Member Author

ixti commented Apr 24, 2018

I'm gonna merge this down and release as v 3.2.1
And then will prepare this patch with extra idea for master branch

@ixti ixti requested a review from tarcieri April 24, 2018 14:50
@tycooon
Copy link
Contributor

tycooon commented Apr 24, 2018

Would be cool to have specs for that.

Webmock triggers `body.each` twice upon inital request recording:

1. to perform request
2. to record request data into webmock request registry

But even without WebMock, generally Body#each must be replayable IMO.
@ixti ixti force-pushed the ixti/fix-form-data-usage-with-webmock branch from 7939b70 to 20d3b67 Compare April 24, 2018 15:28
@ixti
Copy link
Member Author

ixti commented Apr 24, 2018

@tycooon good catch! Thank you! done.

@ixti
Copy link
Member Author

ixti commented Apr 24, 2018

I'm gonna "harden" readable body source validation in master - so that if given io-alike object does not respond to size and rewind - it will fail. Not gonna add it as part of 3.x branch as it's quiet breaking change for people using non-rewindable objects and not using webmock.

UPDATE: Actually not. I just realized that one may want to pass IO.pipe which is readable and good suit (probably) for chunked requests.

@ixti ixti merged commit 2b16fb8 into 3-x-stable Apr 24, 2018
@ixti ixti deleted the ixti/fix-form-data-usage-with-webmock branch April 24, 2018 15:35
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Apr 29, 2018
pkgsrc changes:
- update HOMEPAGE (follow renamed github)

Upstream changes (from CHANGES.md):

## 3.3.0 (2018-04-25)

This version backports some of the fixes and improvements made to development
version of the HTTP gem:

* [#458](httprb/http#458)
  Extract HTTP::Client#build_request method.
  ([@tycoon])


## 3.2.1 (2018-04-24)

* [#468](httprb/http#468)
  Rewind `HTTP::Request::Body#source` once `#each` is complete.
  ([@ixti])


## 3.2.0 (2018-04-22)

This version backports one change we missed to backport in previous release:

* Reduce memory usage when reading response body
  ([@janko-m])


## 3.1.0 (2018-04-22)

This version backports some of the fixes and improvements made to development
version of the HTTP gem:

* Fix for `#readpartial` to respect max length argument.
  ([@janko-m], [@marshall-lee])

* Fix for `HTTP::Request#headline` to allow two leading slashes in path.
  ([@scarfacedeb])

* Fix query string building for string with newlines.
  ([@mikegee])

* Deallocate temporary strings in `Response::Body#to_s`.
  ([@janko-m])

* Add `Request::Body#source`.
  ([@janko-m])
@mikker
Copy link

mikker commented Feb 14, 2019

We're using IO.pipe for chunked POST requests in @elastic's APM Agent and I'm stubbing away rewind on my reader objects because of this.

# HTTP.rb calls #rewind the body stream which IO.pipes don't support
class ModdedIO < IO
  def self.pipe(ext_enc = nil)
    super(ext_enc).tap do |rw|
      rw[0].define_singleton_method(:rewind) { nil }
    end
  end
end

Just wanted to provide an actual use of IO.pipe.

@ixti
Copy link
Member Author

ixti commented Feb 14, 2019

@mikker I have opened an issue with your comment. Let's continue discussion there. We ended up with not requiring IO to respond to rewind:

def validate_source_type!
return if @source.is_a?(String)
return if @source.respond_to?(:read)
return if @source.is_a?(Enumerable)
return if @source.nil?
raise RequestError, "body of wrong type: #{@source.class}"
end

But, I have a feeling that the problem might come from the form data:

https://github.com/httprb/form_data/blob/02800358e4a794e33dfc9cd5308e21ff5efccd2a/lib/http/form_data/composite_io.rb#L56-L60

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jun 7, 2020
Update ruby-http to 4.4.1.


## 4.4.1 (2020-03-29)

* Backport [#590](httprb/http#590)
  Fix parser failing on some edge cases.
  ([@ixti])

## 4.4.0 (2020-03-25)

* Backport [#587](httprb/http#587)
  Fix redirections when server responds with multiple Location headers.
  ([@ixti])

* Backport [#599](httprb/http#599)
  Allow passing HTTP::FormData::{Multipart,UrlEncoded} object directly.
  ([@ixti])

## 4.3.0 (2020-01-09)

* Backport [#581](httprb/http#581)
  Add Ruby-2.7 compatibility.
  ([@ixti], [@janko])


## 4.2.0 (2019-10-22)

* Backport [#489](httprb/http#489)
  Fix HTTP parser.
  ([@ixti], [@fxposter])


## 4.1.1 (2019-03-12)

* Add `HTTP::Headers::ACCEPT_ENCODING` constant.
  ([@ixti])


## 4.1.0 (2019-03-11)

* [#533](httprb/http#533)
  Add URI normalizer feature that allows to swap default URI normalizer.
  ([@mamoonraja])


## 4.0.5 (2019-02-15)

* Backport [#532](httprb/http#532) from master.
  Fix pipes support in request bodies.
  ([@ixti])


## 4.0.4 (2019-02-12)

* Backport [#506](httprb/http#506) from master.
  Skip auto-deflate when there is no body.
  ([@Bonias])


## 4.0.3 (2019-01-18)

* Fix missing URL in response wrapped by auto inflate.
  ([@ixti])

* Provide `HTTP::Request#inspect` method for debugging purposes.
  ([@ixti])


## 4.0.2 (2019-01-15)

* [#506](httprb/http#506)
  Fix instrumentation feature.
  ([@paul])


## 4.0.1 (2019-01-14)

* [#515](httprb/http#515)
  Fix `#build_request` and `#request` to respect default options.
  ([@RickCSong])


## 4.0.0 (2018-10-15)

* [#482](httprb/http#482)
  [#499](httprb/http#499)
  Introduce new features injection API with 2 new feaures: instrumentation
  (compatible with ActiveSupport::Notification) and logging.
  ([@paul])

* [#473](httprb/http#473)
  Handle early responses.
  ([@janko-m])

* [#468](httprb/http#468)
  Rewind `HTTP::Request::Body#source` once `#each` is complete.
  ([@ixti])

* [#467](httprb/http#467)
  Drop Ruby 2.2 support.
  ([@ixti])

* [#436](httprb/http#436)
  Raise ConnectionError when writing to socket fails.
  ([@janko-m])

* [#438](httprb/http#438)
  Expose `HTTP::Request::Body#source`.
  ([@janko-m])

* [#446](httprb/http#446)
  Simplify setting a timeout.
  ([@mikegee])

* [#451](httprb/http#451)
  Reduce memory usage when reading response body.
  ([@janko-m])

* [#458](httprb/http#458)
  Extract HTTP::Client#build_request method.
  ([@tycoon])

* [#462](httprb/http#462)
  Fix HTTP::Request#headline to allow two leading slashes in path.
  ([@scarfacedeb])

* [#454](httprb/http#454)
  [#464](httprb/http#464)
  [#384](httprb/http#384)
  Fix #readpartial not respecting max length argument.
  ([@janko-m], [@marshall-lee])
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.

3 participants