Add specs and fix logic #48
Conversation
Prior to this there were cases where the timeout did not behave as expected.
|
@dmathieu well I did reference @chadbailey59 in #45… |
|
@JackDanger thanks a lot for your work here. As @kch mentioned, I'm sort-of-taking-over on this project here. I have some other specs that I've been planning to merge in, but you've done great stuff here too, so I need to figure out how to smash all of it together. |
| @@ -27,9 +27,10 @@ def call(env) | |||
| request_start = env['HTTP_X_REQUEST_START'] # unix timestamp in ms | |||
| request_start = Time.at(request_start.to_f / 1000) if request_start | |||
| info.age = Time.now - request_start if request_start | |||
| time_left = MAX_REQUEST_AGE - info.age if info.age | |||
| time_left += self.class.overtime if time_left && self.class._request_has_body?(env) | |||
| info.timeout = [self.class.timeout, time_left].compact.select { |n| n >= 0 }.min | |||
There was a problem hiding this comment.
sadly this code is super error prone. in particular, errant values in the HTTP_X_REQUEST_START header can result in a negative time_left, which is why this logic needs to stay here 😿
Sometimes we get times in seconds
|
When will this be merged? |
|
So I wasn't really sure what was getting to me about this patch, but I think I got it now: the thing is
The implication here is that if
The 30s amount is fixed, and that's why it's in a constant. I then explain:
And back in the previous paragraph:
And that's why All that said, the code is clearly confusing, and I think @cjolly is right in that overtime is never used. (Mind you that the overtime stuff never made it to the last beta.) So my thinking now is to refactor a bunch of this, and I hope the tests from this patch and from @chadbailey59 's branch will come in handy. |
|
Now the other problem is that the whole thing around X-Request-Start comes with a lot of assumptions around behaviours from the Heroku router. Which, if you're using nginx, goes down the drain. Most especially, the MAX_REQUEST_AGE thing. As per #38 I wanted to just get a release out, so I might get this out again without overtime support, and work on a more general/flexible implementation next. |
|
Thanks for the update @kch. We've been using this branch in production since I commented above. The big requirement for us (read-heavy app) is that we want a quick timeout for everything but PUTs, POSTs, which shouldn't be subject to the I'm all for getting some tests in place for a refactor and an official release, subsequently addressing the overtime. I'm definitely willing to lend a hand wherever necessary, just let me know. |
|
Right now, even fixing I'm playing with a refactoring that lets you set any of those things to false and then it'll be ignored. As for heroku routing, it drops any request that hasn't had any transfer going in for over 30s. So strictly, it's not about being a POST or PUT, but those do tend to perform transfers while going over 30s, so effectively, it's about them too. |
Some bugs squashed, hopefully none introduced. The intent of code should be much clearer, even more so with all the amazing commentary. Hopefully addresses the issues raised in #48 and #45, plus #50 as a bonus. Not to mention #29 again. @chadbailey59 will probably hate me.
| end | ||
|
|
||
| it "completes successfully" do | ||
| expect { call }.to_not raise_error |
There was a problem hiding this comment.
this test will fail with the new refactor (e599a9f), but I'd say the test is wrong. The refactor should make it more clear the overtime only applies to the wait timeout, not to the service timeout.
Merge remote-tracking branch 'jackdanger/add-specs' into jackdanger-tests Conflicts: lib/rack/timeout.rb
|
At this point I'm closing this. The need for tests is tracked in #52, and the logic has been fixed in the recent refactorings. (mainly e599a9f) Thanks again @JackDanger for the patch, even though it didn't get merged, it was instrumental in the refactor. |
Prior to this there were cases where the timeout did not behave as expected. All tests now pass.