Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Raise WriteError if returning -1 from on_body callback, just like real Curl #273

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants

jure commented Apr 3, 2013

So we have code that goes something like this:

curl.on_body do |data|
  total_size += data.size
  if disallowed_content_type
    # Do this here instead of on-header to allow for redirects
    -1
  elsif total_size > 30_000_000
    -1
  else
    file << data
    data.size
  end
end

And in production (or outside of tests), this raises a Curl::Err::WriteError

req = Curl::Easy.new('http://www.microsoft.com')
=> #<Curl::Easy http://www.microsoft.com>
req.on_body do |data|
-1
end
=> nil
req.perform
Curl::Err::WriteError: Curl::Err::WriteError
  from /Users/jure/.rbenv/versions/2.0.0-p0/lib/ruby/gems/2.0.0/gems/curb-0.8.3/lib/curl/easy.rb:60:in `perform'
  from (irb):6
  from /Users/jure/.rbenv/versions/2.0.0-p0/bin/irb:12:in `<main>'
req.last_result
=> 23

But when using WebMock, there is no WriteError, which makes it impossible to test for those situations.

Curl raises this error from here:
https://github.com/taf2/curb/blob/master/lib/curl/easy.rb#L58-L60

I have no idea how to get a realistic last_result in WebMock (right now it seems to vary wildly, probably because it's not really defined?), but the attached code worked well for my case and is what we're using right now.

I'm happy to debate this further and improve it.

Raise WriteError if returning -1 from on_body callback, just like rea…
…l Curl (Curb)

Whitespace

Add spec for raising WriteError when -1 is returned from on_body
Owner

bblimke commented Apr 4, 2013

@jure thanks for reporting this issue and for the patch.
I would be good to make a proper patch to WebMock that handles all all cases than just error 23 for -1.

jure commented Apr 4, 2013

I'd be glad to do that, but I fear it's currently out of my league to attempt this, or it would at least take a lot of time to attempt this. I'd be happy to get some pointers from you to try and speed things along.

Owner

bblimke commented Sep 27, 2014

Curl has plenty of other errors. Most of the logic is somewhere deep in curl code,
so it would be extremely difficult to replicate everything in WebMock.

Please have a look http://man.cx/curl

Are you sure 23 is generated only on -1 returned? Why would -1 be returned?

jure commented Sep 28, 2014

Oooh, @bblimke, a blast from the past! The on_body callback fails if the return value is not equal to the number of received bytes. We used -1 as it's not possible for a chunk to be -1 bytes long, and so the request failed reliably. We used this reliable failure to control what kind of files we processed (they had to be the right file type and not too big for us to process), and this mechanism was used reliably for millions of files.

Some background on this here: https://github.com/taf2/curb/blob/master/ext/curb_easy.c#L1729-L1734

Though I'm not sure why it says it raises a Curl::Err::AbortedByCallbackError, given that it reliably raised Curl::Err::WriteError for us, and the last changes to that portion of code in curb were made 6 years ago. Update: Oh, and it still raises a WriteError today:

irb(main):001:0> require 'curb'
=> true
irb(main):002:0> req = Curl::Easy.new('http://www.microsoft.com')
=> #<Curl::Easy http://www.microsoft.com>
irb(main):003:0> req.on_body do |data|
irb(main):004:1* -1
irb(main):005:1> end
=> nil
irb(main):006:0> req.perform
Curl::Err::WriteError: Failed writing received data to disk/application
    from /Users/jure/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/curb-0.8.6/lib/curl/easy.rb:72:in `perform'
    from (irb):6
    from /Users/jure/.rbenv/versions/2.1.2/bin/irb:11:in `<main>'

Even though this aspect of Curb looks like it's been working the same for the past few years, I think it's fair to say that you can close the issue, as it's quite difficult to cover all curl's edge cases, as you say.

Owner

bblimke commented Sep 28, 2014

I didn't have much time for WebMock over last year, and I'm now going through the issues and pull requests :)

Thank you very much for taking time and clarifying that!

@bblimke bblimke closed this Sep 28, 2014

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