Bug #11526: Streaming HTTP requests are not idempotent and should not be retried

• Status: Closed
• Assignee: naruse (Yui NARUSE)
• Target version: -
• Backport: 2.0.0: UNKNOWN, 2.1: UNKNOWN, 2.2: UNKNOWN
• Tags: patch, lib

[tdg5 (Danny Guinther)]

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!

[tdg5 (Danny Guinther)]

GitHub PR: https://github.com/ruby/ruby/pull/1019

[tdg5 (Danny Guinther)]

• File trowe-net-http-idempotent-retry-fix.diff trowe-net-http-idempotent-retry-fix.diff added

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.

[tdg5 (Danny Guinther)]

• File trowe-net-http-idempotent-retry-fix-with-test-fixes.diff trowe-net-http-idempotent-retry-fix-with-test-fixes.diff added

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
-"
+""

[jeremyevans0 (Jeremy Evans)]

• Status changed from Open to Assigned
• Assignee set to naruse (Yui NARUSE)

[jeremyevans0 (Jeremy Evans)]

A fix for this has been submitted as an upstream pull request: https://github.com/ruby/net-http/pull/87

[jeremyevans0 (Jeremy Evans)]

• Status changed from Assigned to Closed