Request based workers

[the-allanc]

❓ What kind of change does this PR introduce?

• 🐞 bug fix
• 🐣 feature
• 📋 docs update
• 📋 tests/coverage improvement
• 📋 refactoring
• 💥 other

📋 What is the related issue number (starting with #)

#91

❓ What is the current behavior?

Worker threads are allocated a connection object, and that thread is occupied by that connection is closed or times out.

This can lead to the server having no spare threads to handle new incoming connections - even if there are no further requests coming over the used connections.

❓ What is the new behavior (if this is a feature change)?

We maintain a connection pool of open connections. Cheroot uses select to determine if any of the open connections or the server socket has any incoming data - we then either use an existing connection or accept a new connection, and pass that to the worker thread.

Once the worker thread has processed a request, it will pass the connection back to the connection pool (if it decides that the connection should not be closed). This allow threads to only be occupied by connections that are ready with a request.

📋 Other information:

The connection pool uses two settings - a new one to determine how many keep-alive connections to keep open (this only applies to connections which are not currently being handled by a thread).

It also uses the server socket timeout setting to determine when to close an idle keep-alive connection.

📋 Checklist:

• I think the code is well written
• I wrote good commit messages
• I have squashed related commits together after the changes have been approved
• Unit tests for the changes exist
• Integration tests for the changes exist (if applicable)
• I used the same coding conventions as the rest of the project
• The new code doesn't generate linter offenses
• Documentation reflects the changes
• The PR relates to only one subject with a clear title
  and description in grammatically correct, complete sentences

---

This change is 

[codecov]

Codecov Report

Merging #199 into master will decrease coverage by 0.65%.
The diff coverage is 86.04%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   76.07%   75.41%   -0.66%     
==========================================
  Files          23       24       +1     
  Lines        3636     3685      +49     
==========================================
+ Hits         2766     2779      +13     
- Misses        870      906      +36

[webknjaz]

@the-allanc have you seen https://docs.python.org/3/library/socketserver.html ? Is it applicable to Cheroot project somehow?

[the-allanc]

@the-allanc have you seen https://docs.python.org/3/library/socketserver.html ? Is it applicable to Cheroot project somehow?

I use the term "server socket" as a relic of my days programming in Java -ServerSocket is a class which is used for listening for and accepting incoming connections.

If some other term is appropriate, then please change it. It's not related to the SocketServer class in Python.

[the-allanc]

I've changed the code a bit in a few ways:

1. We store properties on the HTTPConnection object to indicate if it has data ready on the socket, if it's closeable and when it can be expired/
2. We now only expire one connection at a time (the "oldest" one), and let the main Cheroot loop "tick" and to then process that connection.
3. WorkerThreads are now responsible for closing connections, rather than the main Cheroot thread (which is how it used to work previously).

[jaraco]

This implementation looks sound. The structure is good. Kudos for using imperative voice in the docstrings. It's hard for me to tell from the diff how much code is now and how much is re-used, but it feels like just the right amount.

Because of the fundamental nature of cheroot, we're planning to test a private release of this code internally on our most sophisticated CherryPy app. If it succeeds there, I'll have no reservations signing off on the change.

[webknjaz]

@the-allanc FTR I'm having an HTTP parser migration in my mind (#201). How well will this patch play with that?

[webknjaz]

@the-allanc have you seen my question @ #199 (comment)?

[webknjaz]

I believe that @mar10 will be interested in testing this out as well.

[the-allanc]

@the-allanc FTR I'm having an HTTP parser migration in my mind (#201). How well will this patch play with that?

Had a quick look at it, I think generally it should work. Some minor changes required, like the connection manager needing a different way to determine if there's data in the buffer.

@the-allanc have you seen my question @ #199 (comment)?

Yes.

[jaraco]

I've tested this branch on my Windows box and the Python 2.7 errors happen on master same as on the branch, so I believe this branch is safe to merge.

[webknjaz]

Looks like this change introduced the following regressions:

• [cheroot==8.1.0 regression] Spin on Linux when socket filedescriptor is too big for select #249
• [cheroot==8.1.0 regression] Occasional connection resets with concurrent requests #263

[jaraco]

@the-allanc As I imagine you're aware, this change has caused some fairly serious regressions in the stability of cheroot (#249, #263). I suspect they'll be solved by the same investigation. I've spent some time in #263 writing a regression test that captures the failure and expected outcome. Will you have time to investigate these issues in the next few weeks? If not, I may take a stab at it, but if neither of us can make progress on it in fairly short order, I'm going to have to revert this feature.