Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert to IO.select for PerOperation timeouts (fixes #298) #322

Merged
merged 1 commit into from
Mar 21, 2016

Conversation

tarcieri
Copy link
Member

I have tried the repro case from #298 from this and this appears to
fix the problem.

Not sure why: the intended semantics appear to be equivalent. Possibly
a MRI bug that was fixed in 2.3?

I have tried the repro case from #298 from this and this appears to
fix the problem.

Not sure why: the intended semantics appear to be equivalent. Possibly
a MRI bug that was fixed in 2.3?
@tarcieri
Copy link
Member Author

This kind of sucks as these are specifically the timeouts I feel this optimization would be beneficial for. Nevertheless, it seems prior to Ruby 2.3, we're hitting some kind of MRI bug?

/cc @zanker

@zanker
Copy link
Contributor

zanker commented Mar 21, 2016

Lol Ruby 😔

I fee like we just need something for Travis that has very detailed test semantics for each set of timeout code in each Ruby version.

Sent from my iPhone

On Mar 20, 2016, at 18:05, Tony Arcieri notifications@github.com wrote:

This kind of sucks as these are specifically the timeouts I feel this optimization would be beneficial for. Nevertheless, it seems prior to Ruby 2.3, we're hitting some kind of MRI bug?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@zanker
Copy link
Contributor

zanker commented Mar 21, 2016

(Looks good)

Sent from my iPhone

On Mar 20, 2016, at 18:05, Tony Arcieri notifications@github.com wrote:

This kind of sucks as these are specifically the timeouts I feel this optimization would be beneficial for. Nevertheless, it seems prior to Ruby 2.3, we're hitting some kind of MRI bug?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

@tarcieri
Copy link
Member Author

Would be nice if the particular repro case could be turned into some sort of test. That said, timeout tests are the bane of my existence, especially on CI.

All that said... on master (Ruby 2.2.4 / OS X 10.11.3):

$ bundle exec ruby test.rb                                                                                      1 ↵
  1 ... fail (0.05978870391845703)
/Users/bascule/src/http/lib/http/timeout/per_operation.rb:70:in `block in readpartial': Read timed out after 5 seconds (HTTP::TimeoutError)
    from /Users/bascule/src/http/lib/http/timeout/per_operation.rb:61:in `loop'
    from /Users/bascule/src/http/lib/http/timeout/per_operation.rb:61:in `readpartial'
    from /Users/bascule/src/http/lib/http/connection.rb:210:in `read_more'
    from /Users/bascule/src/http/lib/http/connection.rb:84:in `readpartial'
    from /Users/bascule/src/http/lib/http/response/body.rb:48:in `to_s'
    from /Users/bascule/src/http/lib/http/response.rb:79:in `flush'
    from test.rb:11:in `block in <main>'
    from test.rb:6:in `times'
    from test.rb:6:in `<main>'

On this branch:

$ bundle exec ruby test.rb
  1 ... pass (0.4926888942718506)
  2 ... pass (0.07799601554870605)
  3 ... pass (0.06123685836791992)
  4 ... pass (0.07111287117004395)
  5 ... pass (0.0842127799987793)
  6 ... pass (0.07085514068603516)
  7 ... pass (0.0738520622253418)
  8 ... pass (0.07160758972167969)
  9 ... pass (0.06447196006774902)
 10 ... pass (0.0648660659790039)
 11 ... pass (0.07875204086303711)
 12 ... pass (0.10101890563964844)
 13 ... pass (0.06365585327148438)
 14 ... pass (0.06893110275268555)
 15 ... pass (0.06424498558044434)
 16 ... pass (0.0738980770111084)
 17 ... pass (0.0613102912902832)
 18 ... pass (0.059870004653930664)
 19 ... pass (0.06126594543457031)
 20 ... pass (0.06668996810913086)
 21 ... pass (0.06355500221252441)
 22 ... pass (0.06531405448913574)
 23 ... pass (0.06339097023010254)
 24 ... pass (0.06269621849060059)
 25 ... pass (0.06451606750488281)
 26 ... pass (0.06104087829589844)
 27 ... pass (0.0631570816040039)
 28 ... pass (0.0637977123260498)
 29 ... pass (0.06388187408447266)
 30 ... pass (0.07471394538879395)
 31 ... pass (0.0644233226776123)
 32 ... pass (0.1105501651763916)
 33 ... pass (0.06400489807128906)
 34 ... pass (0.06013226509094238)
 35 ... pass (0.06827473640441895)
 36 ... pass (0.07304501533508301)
 37 ... pass (0.07212209701538086)
 38 ... pass (0.0656731128692627)
 39 ... pass (0.07112288475036621)
 40 ... pass (0.08130216598510742)
 41 ... pass (0.06479501724243164)
 42 ... pass (0.07647204399108887)

@tarcieri
Copy link
Member Author

:shipit:

tarcieri added a commit that referenced this pull request Mar 21, 2016
Revert to IO.select for PerOperation timeouts (fixes #298)
@tarcieri tarcieri merged commit bb4d5e1 into master Mar 21, 2016
@tarcieri tarcieri deleted the tarcieri/fix-timeouterror-bug branch March 21, 2016 01:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants