Add between_bytes_timeout to fix timing out when we get a slow request

[darkhelmet]

Description

Closes #2574 by adding a between_bytes_timeout config option to handle slow requests instead of treating first_data_timeout as a global request timeout.

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] or [ci skip] to the pull request title.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

Some notes

• I didn't set a default in const.rb for this since it defaults to whatever is provided for :first_data_timeout
• I added the assert_proper_timeout method in the server test file because I saw another assertion added to that file, there was only one, and I didn't see a rhyme or reason as to where I should add that assertion. Let me know if there's a better spot for it.

[wjordan]

This looks like a great start, thanks for putting this PR together to follow up with this issue!

In order to distinguish between 'first-byte' and 'between-byte' timeouts, not all between-byte timeouts will be set from the reactor-wakeup, there are a couple of other edge cases to consider:

• If queue_requests is false, the connection is never added to the Reactor and will use the loop in Client#finish to read the request fully
• If part of the request is readable on the connection before it's initially accepted (so it's partially read before it ever gets added to the Reactor), the connection will be added to the Reactor with its timeout set from process_client even though it's already past the first byte. (I think this case is what may be causing some of the tests to intermittently fail)

One possible way of handling these cases would be to accept a second between_bytes_timeout parameter for both Client#set_timeout and Client#finish methods and use the second timeout if @parsed_bytes == 0.

That said, given the slightly greater amount of changes needed, it might be worth splitting it into two separate PRs, one to narrowly fix the regression on first_data_timeout behavior and another to add the between_bytes_timeout option. @nateberkopec any thoughts on direction to go with this (and thoughts on whether a separate between_bytes_timeout option would be useful in general)?

[darkhelmet]

I got a test and change for when queue_requests = false but I'm not sure how to write a test for your other point. (edit: hmmm...)

Yeah, we can definitely split this up if you want, or do whatever other refactorings. Just let me know what direction I should go with it.

[darkhelmet]

Well, now I'm just breaking things. I'll wait for some guidance :)

[nateberkopec]

Sorry @darkhelmet, I've been busy. I or someone else will be around to help soon.

[dchandekstark]

Just commenting to bump the issue. Thanks to @darkhelmet for reporting and working on a solution. We got bit by #2574 using version 5.0.3 exactly. Will likely downgrade to 5.0.2 until this PR is sorted out and released.

[wjordan]

I made some tweaks to the PR in 6050b4f, which fixes up the logic on some of the edge cases so all the tests pass. It also updates test_no_timeout_after_data_received to match the case described in the issue making it a proper 'regression test' where it fails without this PR and passes with it, to make sure we don't unintentionally reintroduce the same bug again later on.

@darkhelmet please take a look! If the extra commit looks good to you feel free merge the commit into your branch to update this PR.

[nateberkopec]

@darkhelmet Looks you need to rebase on top of master

[nateberkopec]

Code LGTM

[nateberkopec]

@darkhelmet Where exactly do you need further help or direction here?

[darkhelmet]

@nateberkopec I haven't had time to look at this further. With #2606 the main problem is fixed as well, so the need isn't as great. If you want to get this PR out of sight, we can close it and I can revisit it in the future if I find the time.

[nateberkopec]

Cool! Happy to help when you've got the time. 👍