Streaming requests shouldn't be idempotent

[ghost]

I posted an issue on Excon's github page for this same problem (excon/excon#530), but I think it's important to also post it here as Fog hardcodes the idempotent option to true for a lot of requests and doesn't seem to allow the option to be customized (see, for instance, https://github.com/fog/fog-aws/blob/master/lib/fog/aws/requests/storage/get_object.rb#L55).

The idempotent option can be problematic for streaming requests since Excon automatically retries the entire request from the beginning when something goes wrong, without making it clear that it's doing so. This can easily result in data corruption.

Consider the following example, using Fog, demonstrating this problem

storage = Fog::Storage.new({
  :aws_access_key_id=>"YOUR_ACCESS_KEY_ID",
  :aws_secret_access_key=>"YOUR_SECRET_ACCESS_KEY",
  :provider=>"AWS",
  :region=>"us-east-1"
})
bucket = "YOUR_BUCKET"
filename = "YOUR_SUFFICIENTLY_LARGE_FILE"

expected_bytes = nil
repeated = false
chunks = []
storage.get_object(bucket, filename, {}) do |chunk, remaining_bytes, total_bytes|
  expected_bytes ||= total_bytes
  chunks << chunk
  unless repeated
    repeated = true
    raise Excon::Errors::Timeout, "Boom"
  end
end
downloaded_bytes = chunks.inject(0){ |sum, chunk| sum += chunk.length } # => 99417360
expected_bytes # => 98368784
chunks[0] == chunks[1] # => true
chunks[1..-1].inject(0){ |sum, chunk| sum += chunk.length } == expected_bytes # => true 

[geemus]

I commented over on the excon issue (excon/excon#530). In summary, I agree the interface leaves something to be desired in terms of understanding the state of retries, but having excon itself change the idempotent flag based on presence of block seems unexpected and limiting. That said, I suspect a better fix at least in this case would be to disable idempotent on get_object when a block is passed. We could also consider making it an option, as you suggested, but I suspect just turning it off will be easiest and give the best experience. What do you think?

[ghost]

I think that makes sense. I'm in favor of just turning it off for now and maybe sometime in the future it could become configurable.

[geemus]

Agreed, so I think we should leave it on for non-streaming and turn off when a block is supplied, right? Would you be up for making a quick pull request with that change? Thanks!

[ghost]

Here's the pull request: #183

[geemus]

Thanks!