Non-blocking socket support/better handling of HTTP/1.1 connections

[jaraco]

❓ 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? (You can also link to an open issue here)

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

This is a re-submit of #117 with some subsequent improvements.

📋 Other information:

📋 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 #176 into master will increase coverage by 0.45%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
+ Coverage   71.46%   71.92%   +0.45%     
==========================================
  Files          23       23              
  Lines        3557     3601      +44     
==========================================
+ Hits         2542     2590      +48     
+ Misses       1015     1011       -4

[jaraco]

I've published a pre-release version of this PR in my devpi repo as cheroot-6.5.6.dev28+g9413ed9c.

[the-allanc]

I don't think this patch even works as is.

Using this basic setup:

import cherrypy

class Server:
    @classmethod
    def run(cls):
        config = {
            'global': {
                'server.socket_host': '::',
                'server.socket_port': 56951,
                'server.socket_timeout': 0,
                'server.thread_pool': 2,
            }
        }

        cherrypy.config.update(config)
        cherrypy.engine.start()
        cherrypy.tree.mount(cls(), '/')
        cherrypy.engine.block()

    @cherrypy.expose
    def index(self):
        return "Hello World!"

__name__ == '__main__' and Server.run()

And then running this in a separate Python prompt:

Python 2.7.12 (default, Nov 12 2018, 14:36:49) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> s = [requests.Session() for i in range(3)]
>>> url = 'http://localhost:56951/'
>>> r0 = s[0].get(url)
>>> r1 = s[1].get(url)
>>> r2 = s[2].get(url)

This causes the final line to just hang, which seems to indicate that the keep-alive connections are still occupying threads.

[the-allanc]

After looking at this for about a day and a half, I'm not convinced this is the right way of dealing with this issue (even if we corrected the issue). We're conflating multiple things at the same time.

The initial reporting of this issue was to do with some requests encountering a delay of 10 seconds before being dealt with. The ideal way that this should be dealt with is to change the behaviour of the threadpool, so that threads work with requests, not connections.

But the fix that we have here works around the problem, by focusing on the behaviour we have with keep-alive connections, and encourages the use of non-blocking sockets as a way to address the issue. It doesn't quite bring us request-based worker threads, but it does give us threads with a one-to-many relationship with connections.

This does improve the situation in some conditions, but not without pitfalls:

• A secondary request coming in via a Keep-Alive session may have to wait if the thread that owns that connection is currently dealing with an incoming request from another connection. This could even happen if there are free threads.
• It makes thread provisioning much more difficult. If a thread owns a keep-alive connection, then is it ever really "idle"? It has to continuously check the connections that it owns.

I don't think we should continue with work on this branch - it's a hack (which is an improvement), but it then starts conflating and confusing issues. I'm going to bring up the separate issues we should focus on in further comments.

[the-allanc]

Better handling of HTTP/1.1 connections

Cheroot does a decent job when it comes to managing HTTP/1.1 connections (I believe). I think the issue is that the default socket timeout is too long for a Keep-Alive connection.

So as an alternative, how about having a separate socket timeout for keep-alive connections? We would have to consider timeouts specified in the request itself.

Non-blocking sockets

Why should cheroot provide support for non-blocking sockets? Either cheroot can presume sockets are blocking (and can set and capture timeouts), or it should presume will sockets are non-blocking, and contain its own logic to expire connections. I don't think it's reasonable to expect cheroot to be able to handle both.

There might be a call to allow sockets to be available and non-blocking once the request has been read in (but perhaps the body hasn't). In that case, we could consider allowing timeout behaviour to be different between the point that cheroot is handling a request, and when the request is passed on to the WSGI app.

Request-based workers

This would be a more fundamental change. We would have worker threads to handle individual requests, and then a separate thread pool to manage both incoming connections and current keep-alive connections, and it would be responsible to determine how many connections it would permit to have around. This would probably require having new settings to indicate how many connections would be available, and then deciding if the existing "thread pool" settings still relate to connections or to requests.

[jaraco]

Thank you @the-allanc for your critical review and insight. You've got a tighter grasp on this issue than any one else. If you believe this change is inadequate or incorrect, then I'm inclined to agree.

I'm having difficulty disentangling these issues, as you've attempted to do. Our key motivations here are to find a way to robustly handle HTTP 1.1 connections under load, and that using non-blocking sockets would achieve that. cherrypy/cherrypy#1764 was another manifestation of the issue. So we're not wed to supporting non-blocking sockets, but to devising the most robust solution we can afford to implement.

I like your proposal of request-based workers. Would you be willing to draft a proposal?

[webknjaz]

I completely agree with @the-allanc's assessments. I just want to add that HTTP/1.1 pipelining seems to be a bit broken: #69.

[the-allanc]

I've created #199 as an alternative fix.

[webknjaz]

Closing in favor or #199.