Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Curl::Multi.http doesn't actually call perform when :max_connects >= number of requests #115

Merged
merged 5 commits into from

2 participants

@chuyeow

This bug had me stumped when I was writing a Faraday adapter for Curb - I could never get the parallel requests test to pass!

I checked Curb's own tests but the tc_curl_multi.rb tests were still passing - it was only after I explicitly broke the tests (see e45a394) and the tests still passed that I realized the callbacks weren't actually being performed, so no assertions were actually run.

I traced the error back to 327e209, circa Curb 0.7.14 (it's still working on 0.7.14, but not in any versions after that). It's all due to the until urls_with_config.empty? check (I find until to be rather confusing).

Fixed the problem, plus added a test that covers the case where :max_connects < number of requests.

Note: You can leave out the commits I added to break and fix the tests (e45a394, fdea6ab) - I left them in for verification.

@taf2
Owner

i'll try and review this one over this weekend.

@chuyeow

Hi Todd, I hate to be naggy but I hope you can merge this (this weekend maybe?) and publish a new gem release soon. I don't know how the bug slipped past so many releases so please verify my findings in case I've got it wrong. My guess is that most people are using Curl::Multi#add and Curl::Multi#perform instead of the simpler Curl::Multi.verb methods - or there're just not many people actually using Curb's multi interface.

@taf2 taf2 merged commit dceafcd into taf2:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 10, 2012
  1. @chuyeow

    Break Curl::Multi get, post, download and http tests to show that the…

    chuyeow authored
    …y still pass because the callbacks are never actually run.
  2. @chuyeow

    Fix Curl::Multi not actually performing HTTP requests in cases when t…

    chuyeow authored
    …he :max_connects >= number of requests.
  3. @chuyeow

    Revert "Break Curl::Multi get, post, download and http tests to show …

    chuyeow authored
    …that they still pass because the callbacks are never actually run."
    
    This reverts commit e45a394.
  4. @chuyeow
  5. @chuyeow

    Fix typo.

    chuyeow authored
This page is out of date. Refresh to see the latest.
Showing with 28 additions and 5 deletions.
  1. +8 −4 lib/curl/multi.rb
  2. +20 −1 tests/tc_curl_multi.rb
View
12 lib/curl/multi.rb
@@ -157,13 +157,17 @@ def http(urls_with_config, multi_options={}, &blk)
end
end
- until urls_with_config.empty?
- m.perform do
+ if urls_with_config.empty?
+ m.perform
+ else
+ until urls_with_config.empty?
+ m.perform do
+ consume_free_handles.call
+ end
consume_free_handles.call
end
- consume_free_handles.call
+ free_handles = nil
end
- free_handles = nil
end
# call-seq:
View
21 tests/tc_curl_multi.rb
@@ -385,7 +385,26 @@ def test_multi_easy_http_01
end
end
- def test_mutli_recieves_500
+ def test_multi_easy_http_with_max_connects
+ urls = [
+ { :url => TestServlet.url + '?q=1', :method => :get },
+ { :url => TestServlet.url + '?q=2', :method => :get },
+ { :url => TestServlet.url + '?q=3', :method => :get }
+ ]
+ Curl::Multi.http(urls, {:pipeline => true, :max_connects => 1}) do|easy, code, method|
+ assert_equal nil, code
+ case method
+ when :post
+ assert_match /POST/, easy.body_str
+ when :get
+ assert_match /GET/, easy.body_str
+ when :put
+ assert_match /PUT/, easy.body_str
+ end
+ end
+ end
+
+ def test_multi_recieves_500
m = Curl::Multi.new
e = Curl::Easy.new("http://127.0.0.1:9129/methods")
failure = false
Something went wrong with that request. Please try again.