Project

General

Profile

Bug #11526

Streaming HTTP requests are not idempotent and should not be retried

Added by tdg5 (Danny Guinther) over 3 years ago. Updated over 3 years ago.

Status:
Open
Priority:
Normal
Assignee:
-
Target version:
-
[ruby-core:<unknown>]

Description

Hello!

A colleague of mine ran into an issue today where he discovered that streaming HTTP requests made with Net::HTTP would retry some types of errors without giving any indication that an error had occurred and the request stream had been rewound. This ultimately resulted in a response body that contained an incomplete response followed by the complete response body.

I think when the request is being streamed it cannot be considered idempotent because there's no telling what sort of side effects might occur in the block that the response chunk is yielded to.

I've attached a diff and will create a GitHub PR shortly that aims to deal with this behavior by only retrying the request if a block is not given.

That said, I suspect this issue warrants further discussion.

We are not the first to run into this issue. One of the developers of the aws-sdk gem ran into this same issue in the last 6 months and chose to handle the issue by clearing out the IDEMPOTENT_METHODS_ collection such that no requests would automatically retry. This seems like overkill to me, but makes sense for a minimally evasive monkeypatch.

The author of that patch, Trevor Rowe, suggested that he would create an issue here, but I have been unable to find such an issue.

The related aws-sdk GitHub issue: https://github.com/aws/aws-sdk-ruby/pull/799
The related aws-sdk GitHub commit: https://github.com/aws/aws-sdk-ruby/commit/5a005974afcda0ec0c8e9332bed4c70a78443500

If I can provide any further information on this matter, please let me know.

Thanks in advance for any and all help!


Files

no_retry_http_streams.diff (2.3 KB) no_retry_http_streams.diff Patch that changes Net::HTTP behavior to only retry the request if a block is not given tdg5 (Danny Guinther), 09/14/2015 07:12 PM
trowe-net-http-idempotent-retry-fix.diff (6.52 KB) trowe-net-http-idempotent-retry-fix.diff tdg5 (Danny Guinther), 09/15/2015 05:40 PM
trowe-net-http-idempotent-retry-fix-with-test-fixes.diff (7.42 KB) trowe-net-http-idempotent-retry-fix-with-test-fixes.diff tdg5 (Danny Guinther), 09/15/2015 08:00 PM

History

#2

Updated by tdg5 (Danny Guinther) over 3 years ago

Trevor Rowe (https://github.com/trevorrowe) also proposed the following PR on GitHub to address this issue on June 30th, 2015: https://github.com/ruby/ruby/pull/951

His code looks solid to me and seems a better solution than what I've hacked together. As such, I have closed my PR with a reference to Trevor's PR.

If Trevor's PR was ignored because a respective ticket was not filed, please consider this issue to be the missing issue and let's get his code merged!

I've created a branch of Trevor's change rebased against current trunk here: https://github.com/tdg5/ruby/tree/trowe-net-http-idempotent-retry-fix

I've also attached a diff of Trevor's change (rebased against current trunk) to this update.

#3

Updated by tdg5 (Danny Guinther) over 3 years ago

Attaching a new diff based on Trevor's branch with a few additional test fixes.

All HTTP-related tests pass, but seems like something in RDoc may be broken in trunk:

  1) Failure:
TestRDocMarkupPreProcess#test_include_file_in_other_directory [/home/danny/src/ruby-trunk/test/rdoc/test_rdoc_markup_pre_process.rb:84]:
--- expected
+++ actual
@@ -1,2 +1 @@
-"test file
-"
+""

Also available in: Atom PDF