From 6841b6b5354d8705bf9c93718496646cdbdbbff8 Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Thu, 13 Aug 2020 08:50:06 -0700
Subject: [PATCH 01/16] connections: copy selector map before modifying

---
 cheroot/connections.py | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/cheroot/connections.py b/cheroot/connections.py
index 14c66dbea0..7044cc5897 100644
--- a/cheroot/connections.py
+++ b/cheroot/connections.py
@@ -4,6 +4,7 @@
 __metaclass__ = type
 
 import collections
+import copy
 import io
 import os
 import socket
@@ -101,7 +102,8 @@ def expire(self):
         threshold = time.time() - self.server.timeout
         timed_out_connections = (
             (sock_fd, conn)
-            for _, (_, sock_fd, _, conn) in self._selector.get_map().items()
+            for _, (_, sock_fd, _, conn)
+            in copy.copy(self._selector.get_map()).items()
             if conn != self.server and conn.last_used < threshold
         )
         for sock_fd, conn in timed_out_connections:
@@ -137,7 +139,7 @@ def get_conn(self):  # noqa: C901  # FIXME
             ]
         except OSError:
             # Mark any connection which no longer appears valid
-            for _, key in self._selector.get_map().items():
+            for _, key in copy.copy(self._selector.get_map()).items():
                 # If the server socket is invalid, we'll just ignore it and
                 # wait to be shutdown.
                 if key.data == self.server:
@@ -266,7 +268,7 @@ def close(self):
             conn.close()
         self._readable_conns.clear()
 
-        for _, key in self._selector.get_map().items():
+        for _, key in copy.copy(self._selector.get_map()).items():
             if key.data != self.server:  # server closes its own socket
                 key.data.socket.close()
 

From 7c885539593f2056f376850ed475df3509947b00 Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Thu, 13 Aug 2020 13:20:49 -0700
Subject: [PATCH 02/16] connections: do not modify selector during iteration

---
 cheroot/connections.py | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/cheroot/connections.py b/cheroot/connections.py
index 7044cc5897..359cba1850 100644
--- a/cheroot/connections.py
+++ b/cheroot/connections.py
@@ -4,7 +4,6 @@
 __metaclass__ = type
 
 import collections
-import copy
 import io
 import os
 import socket
@@ -100,12 +99,12 @@ def expire(self):
         # find any connections still registered with the selector
         # that have not been active recently enough.
         threshold = time.time() - self.server.timeout
-        timed_out_connections = (
+        timed_out_connections = [
             (sock_fd, conn)
             for _, (_, sock_fd, _, conn)
-            in copy.copy(self._selector.get_map()).items()
+            in self._selector.get_map().items()
             if conn != self.server and conn.last_used < threshold
-        )
+        ]
         for sock_fd, conn in timed_out_connections:
             self._selector.unregister(sock_fd)
             conn.close()
@@ -139,7 +138,8 @@ def get_conn(self):  # noqa: C901  # FIXME
             ]
         except OSError:
             # Mark any connection which no longer appears valid
-            for _, key in copy.copy(self._selector.get_map()).items():
+            invalid_entries = []
+            for _, key in self._selector.get_map().items():
                 # If the server socket is invalid, we'll just ignore it and
                 # wait to be shutdown.
                 if key.data == self.server:
@@ -148,10 +148,11 @@ def get_conn(self):  # noqa: C901  # FIXME
                 try:
                     os.fstat(key.fd)
                 except OSError:
-                    # Socket is invalid, close the connection
-                    self._selector.unregister(key.fd)
-                    conn = key.data
-                    conn.close()
+                    invalid_entries.append((key.fd, key.data))
+
+            for sock_fd, conn in invalid_entries:
+                self._selector.unregister(sock_fd)
+                conn.close()
 
             # Wait for the next tick to occur.
             return None
@@ -268,7 +269,7 @@ def close(self):
             conn.close()
         self._readable_conns.clear()
 
-        for _, key in copy.copy(self._selector.get_map()).items():
+        for _, key in self._selector.get_map().items():
             if key.data != self.server:  # server closes its own socket
                 key.data.socket.close()
 

From 639a65989a3136c8f24dd1242f79b2ecdf402a15 Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Fri, 14 Aug 2020 09:06:05 -0700
Subject: [PATCH 03/16] server: don't use connections after it's closed

---
 cheroot/server.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/cheroot/server.py b/cheroot/server.py
index 855e04ccb5..4c6bffbc54 100644
--- a/cheroot/server.py
+++ b/cheroot/server.py
@@ -1162,7 +1162,8 @@ def send_headers(self):  # noqa: C901  # FIXME
         # Override the decision to not close the connection if the connection
         # manager doesn't have space for it.
         if not self.close_connection:
-            can_keep = self.server.connections.can_add_keepalive_connection
+            can_keep = self.ready \
+                and self.server.connections.can_add_keepalive_connection
             self.close_connection = not can_keep
 
         if b'connection' not in hkeys:

From 8ea979589f9f98b880273d75d543d78c9bd1317b Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Fri, 14 Aug 2020 09:12:49 -0700
Subject: [PATCH 04/16] server: parens rather than backslash for multiline expr

---
 cheroot/server.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/cheroot/server.py b/cheroot/server.py
index 4c6bffbc54..80d977e557 100644
--- a/cheroot/server.py
+++ b/cheroot/server.py
@@ -1162,8 +1162,10 @@ def send_headers(self):  # noqa: C901  # FIXME
         # Override the decision to not close the connection if the connection
         # manager doesn't have space for it.
         if not self.close_connection:
-            can_keep = self.ready \
+            can_keep = (
+                self.ready
                 and self.server.connections.can_add_keepalive_connection
+            )
             self.close_connection = not can_keep
 
         if b'connection' not in hkeys:

From ae8ad4060b10d9f79fb8bba093261f63f347907b Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Fri, 14 Aug 2020 09:17:05 -0700
Subject: [PATCH 05/16] server: ensure server is ready, rather than request

---
 cheroot/server.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cheroot/server.py b/cheroot/server.py
index 80d977e557..42decc2a7c 100644
--- a/cheroot/server.py
+++ b/cheroot/server.py
@@ -1163,7 +1163,7 @@ def send_headers(self):  # noqa: C901  # FIXME
         # manager doesn't have space for it.
         if not self.close_connection:
             can_keep = (
-                self.ready
+                self.server.ready
                 and self.server.connections.can_add_keepalive_connection
             )
             self.close_connection = not can_keep

From 853706e6e3667e2a8a10a3c2d2781f7c0b26e34c Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sat, 22 Aug 2020 20:54:31 -0500
Subject: [PATCH 06/16] Monkeypatch the server error_log method to verify the
 content during tests.

Using the new error_log mock class inspect the value during the keep
alive tests, previously ignoring the log caused the regression of
issue #313.
---
 cheroot/test/test_conn.py | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 33a644b53d..92ba8d1cc5 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -5,6 +5,9 @@
 
 import socket
 import time
+import logging
+import traceback as traceback_
+from collections import namedtuple
 
 from six.moves import range, http_client, urllib
 
@@ -108,7 +111,7 @@ def _munge(string):
 
 
 @pytest.fixture
-def testing_server(wsgi_server_client):
+def testing_server(wsgi_server_client, monkeypatch):
     """Attach a WSGI app to the given server and preconfigure it."""
     app = Controller()
 
@@ -121,6 +124,27 @@ def _timeout(req, resp):
     wsgi_server.timeout = timeout
     wsgi_server.server_client = wsgi_server_client
     wsgi_server.keep_alive_conn_limit = 2
+
+    # patch the error_log calls of the server instance
+    class ErrorLog:
+        """Mock class to access the server error_log calls made by the server.
+        """
+        FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
+
+        def __init__(self):
+            self.calls = []
+
+        def __call__(self, msg='', level=20, traceback=False):
+            if traceback:
+                tblines = traceback_.format_exc()
+            else:
+                tblines = ''
+            self.calls.append(
+                ErrorLog.FuncCall(msg, level, tblines)
+            )
+
+    monkeypatch.setattr(wsgi_server, 'error_log', ErrorLog())
+
     return wsgi_server
 
 
@@ -481,6 +505,17 @@ def request(conn, keepalive=True):
     # But the second one should still be valid.
     request(c2)
 
+    # Verify the server error log, just case something happened
+    # when expiring connections.
+    # In particular, verify that we didn't have any call with level
+    # logging.ERROR.
+    for c in test_client.server_instance.error_log.calls:
+        assert c.level != logging.ERROR, (
+            "Found error in the error log: \n"
+            "message = '{}'\n"
+            "{}".format(c.msg, c.traceback)
+        )
+
     # Restore original timeout.
     test_client.server_instance.timeout = timeout
 

From 7134c90a23e0c984b71ff71fc5d64fd814d335af Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 01:00:11 -0500
Subject: [PATCH 07/16] Generalize the error_log verification for the
 testing_server fixture.

---
 cheroot/test/test_conn.py | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 92ba8d1cc5..92719661d4 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -133,8 +133,10 @@ class ErrorLog:
 
         def __init__(self):
             self.calls = []
+            # to be used the the teardown validation
+            self.ignored_msgs = []
 
-        def __call__(self, msg='', level=20, traceback=False):
+        def __call__(self, msg='', level=logging.INFO, traceback=False):
             if traceback:
                 tblines = traceback_.format_exc()
             else:
@@ -144,8 +146,17 @@ def __call__(self, msg='', level=20, traceback=False):
             )
 
     monkeypatch.setattr(wsgi_server, 'error_log', ErrorLog())
-
-    return wsgi_server
+    yield wsgi_server
+    # Teardown verification, in case that the server logged an
+    # error that wasn't notified to the client or we just made a mistake.
+    for c in wsgi_server.error_log.calls:
+        if c.level > logging.WARNING:
+            if c.msg not in wsgi_server.error_log.ignored_msgs:
+                raise AssertionError(
+                    "Found error in the error log: "
+                    "message = '{}', level = '{}'\n"
+                    "{}".format(c.msg, c.level, c.traceback)
+                )
 
 
 @pytest.fixture
@@ -505,17 +516,6 @@ def request(conn, keepalive=True):
     # But the second one should still be valid.
     request(c2)
 
-    # Verify the server error log, just case something happened
-    # when expiring connections.
-    # In particular, verify that we didn't have any call with level
-    # logging.ERROR.
-    for c in test_client.server_instance.error_log.calls:
-        assert c.level != logging.ERROR, (
-            "Found error in the error log: \n"
-            "message = '{}'\n"
-            "{}".format(c.msg, c.traceback)
-        )
-
     # Restore original timeout.
     test_client.server_instance.timeout = timeout
 
@@ -983,8 +983,13 @@ def test_Content_Length_out(
 
     assert actual_status == expected_resp_status
     assert actual_resp_body == expected_resp_body
-
     conn.close()
+    # the server logs the exception that we had verified from the
+    # client perspective. Tell the error_log verification that
+    # it can ignore that message.
+    test_client.server_instance.error_log.ignored_msgs.append(
+        "ValueError('Response body exceeds the declared Content-Length.')"
+    )
 
 
 @pytest.mark.xfail(

From 3daf3e01f633a95b03b3fe61ae77f82fb124990f Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 14:08:10 -0500
Subject: [PATCH 08/16] Add test to verify the error handling segment of
 ConnectionManager.get_conn

---
 cheroot/test/test_conn.py | 73 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 92719661d4..845cacb837 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -17,6 +17,7 @@
 
 from cheroot.test import helper, webtest
 from cheroot._compat import IS_PYPY
+import cheroot.server
 
 
 timeout = 1
@@ -1040,3 +1041,75 @@ def test_No_CRLF(test_client, invalid_terminator):
     expected_resp_body = b'HTTP requires CRLF terminators'
     assert actual_resp_body == expected_resp_body
     conn.close()
+
+
+def test_invalid_selected_connection(test_client, monkeypatch):
+    """
+    Test the error handling segment of
+    ``cheroot.connections.ConnectionManager.get_conn``,
+    using the new connection handling based on the selectors module.
+    """
+    class FaultySelect:
+        def __init__(self, original_select):
+            self.original_select = original_select
+            self.request_served = False
+            self.os_error_triggered = False
+
+        def __call__(self, timeout):
+            if self.request_served:
+                self.os_error_triggered = True
+                raise OSError("Error while selecting the client socket.")
+            else:
+                return self.original_select(timeout)
+
+    class FaultyGetMap:
+        def __init__(self, original_get_map):
+            self.original_get_map = original_get_map
+            self.sabotage_conn = False
+            self.socket_closed = False
+
+        def __call__(self):
+            if self.sabotage_conn:
+                for _, (*_, conn) in self.original_get_map().items():
+                    if isinstance(conn, cheroot.server.HTTPConnection):
+                        # close the socket to cause OSError
+                        conn.close()
+                        self.socket_closed = True
+            return self.original_get_map()
+
+    # patch the select method
+    faux_select = FaultySelect(
+        test_client.server_instance.connections._selector.select
+    )
+    monkeypatch.setattr(
+        test_client.server_instance.connections._selector,
+        'select',
+        faux_select
+    )
+
+    # patch the get_map method
+    faux_get_map = FaultyGetMap(
+        test_client.server_instance.connections._selector.get_map
+    )
+
+    monkeypatch.setattr(
+        test_client.server_instance.connections._selector,
+        'get_map',
+        faux_get_map
+    )
+
+    # request a page with connection keep-alive to make sure
+    # we'll have a connection to be modified.
+    resp_status, resp_headers, resp_body = test_client.request(
+        "/page1", headers=[('Connection', 'Keep-Alive'), ],
+    )
+
+    assert resp_status == '200 OK'
+    # trigger the internal errors
+    faux_get_map.sabotage_conn = faux_select.request_served = True
+    # give time to make sure the error gets handled
+    time.sleep(0.2)
+    assert faux_select.os_error_triggered
+    assert faux_get_map.socket_closed
+    # any error in the error handling should be catched by the
+    # teardown verification for the error_log

From 738fcdfb45db718d7b9b8fe024473435701b224e Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 14:39:01 -0500
Subject: [PATCH 09/16] Conform to the pre-commit desires.

---
 cheroot/test/test_conn.py | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 845cacb837..035bc59bfe 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -142,9 +142,7 @@ def __call__(self, msg='', level=logging.INFO, traceback=False):
                 tblines = traceback_.format_exc()
             else:
                 tblines = ''
-            self.calls.append(
-                ErrorLog.FuncCall(msg, level, tblines)
-            )
+            self.calls.append(ErrorLog.FuncCall(msg, level, tblines))
 
     monkeypatch.setattr(wsgi_server, 'error_log', ErrorLog())
     yield wsgi_server
@@ -154,9 +152,9 @@ def __call__(self, msg='', level=logging.INFO, traceback=False):
         if c.level > logging.WARNING:
             if c.msg not in wsgi_server.error_log.ignored_msgs:
                 raise AssertionError(
-                    "Found error in the error log: "
+                    'Found error in the error log: '
                     "message = '{}', level = '{}'\n"
-                    "{}".format(c.msg, c.level, c.traceback)
+                    '{}'.format(c.msg, c.level, c.traceback),
                 )
 
 
@@ -989,7 +987,7 @@ def test_Content_Length_out(
     # client perspective. Tell the error_log verification that
     # it can ignore that message.
     test_client.server_instance.error_log.ignored_msgs.append(
-        "ValueError('Response body exceeds the declared Content-Length.')"
+        "ValueError('Response body exceeds the declared Content-Length.')",
     )
 
 
@@ -1058,7 +1056,7 @@ def __init__(self, original_select):
         def __call__(self, timeout):
             if self.request_served:
                 self.os_error_triggered = True
-                raise OSError("Error while selecting the client socket.")
+                raise OSError('Error while selecting the client socket.')
             else:
                 return self.original_select(timeout)
 
@@ -1079,29 +1077,29 @@ def __call__(self):
 
     # patch the select method
     faux_select = FaultySelect(
-        test_client.server_instance.connections._selector.select
+        test_client.server_instance.connections._selector.select,
     )
     monkeypatch.setattr(
         test_client.server_instance.connections._selector,
         'select',
-        faux_select
+        faux_select,
     )
 
     # patch the get_map method
     faux_get_map = FaultyGetMap(
-        test_client.server_instance.connections._selector.get_map
+        test_client.server_instance.connections._selector.get_map,
     )
 
     monkeypatch.setattr(
         test_client.server_instance.connections._selector,
         'get_map',
-        faux_get_map
+        faux_get_map,
     )
 
     # request a page with connection keep-alive to make sure
     # we'll have a connection to be modified.
     resp_status, resp_headers, resp_body = test_client.request(
-        "/page1", headers=[('Connection', 'Keep-Alive'), ],
+        '/page1', headers=[('Connection', 'Keep-Alive')],
     )
 
     assert resp_status == '200 OK'

From e9c2e1ba779fd7ba49332474c79ac519522c034c Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 14:51:31 -0500
Subject: [PATCH 10/16] More conforming to the pre-commit desires.

---
 cheroot/test/test_conn.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 035bc59bfe..2f7c2a6db7 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -128,8 +128,7 @@ def _timeout(req, resp):
 
     # patch the error_log calls of the server instance
     class ErrorLog:
-        """Mock class to access the server error_log calls made by the server.
-        """
+        """Mock class to access the server error_log calls made by the server."""
         FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
 
         def __init__(self):
@@ -1042,9 +1041,7 @@ def test_No_CRLF(test_client, invalid_terminator):
 
 
 def test_invalid_selected_connection(test_client, monkeypatch):
-    """
-    Test the error handling segment of
-    ``cheroot.connections.ConnectionManager.get_conn``,
+    """Test the error handling segment of ``cheroot.connections.ConnectionManager.get_conn``,
     using the new connection handling based on the selectors module.
     """
     class FaultySelect:

From 92ca21ebe752f9622ca4490babb3fe103680bd4e Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 15:06:17 -0500
Subject: [PATCH 11/16] Ignore some of the pycodestyle checks

---
 cheroot/test/test_conn.py | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 2f7c2a6db7..501513d86b 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -127,8 +127,11 @@ def _timeout(req, resp):
     wsgi_server.keep_alive_conn_limit = 2
 
     # patch the error_log calls of the server instance
-    class ErrorLog:
-        """Mock class to access the server error_log calls made by the server."""
+    class ErrorLog:  # noqa: D200
+        """
+        Mock class to access the server error_log calls made by the server.
+        """
+
         FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
 
         def __init__(self):
@@ -1040,9 +1043,9 @@ def test_No_CRLF(test_client, invalid_terminator):
     conn.close()
 
 
-def test_invalid_selected_connection(test_client, monkeypatch):
-    """Test the error handling segment of ``cheroot.connections.ConnectionManager.get_conn``,
-    using the new connection handling based on the selectors module.
+def test_invalid_selected_connection(test_client, monkeypatch):  # noqa: D200,D205,D400,E501
+    """Test the error handling segment of
+    ``cheroot.connections.ConnectionManager.get_conn``.
     """
     class FaultySelect:
         def __init__(self, original_select):

From f8a24a3861ceecd1cc7bde506ed5b6ad9dd11972 Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 15:59:53 -0500
Subject: [PATCH 12/16] Apply some suggestions from code review

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
---
 cheroot/test/test_conn.py | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 501513d86b..26b3dda512 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -151,13 +151,14 @@ def __call__(self, msg='', level=logging.INFO, traceback=False):
     # Teardown verification, in case that the server logged an
     # error that wasn't notified to the client or we just made a mistake.
     for c in wsgi_server.error_log.calls:
-        if c.level > logging.WARNING:
-            if c.msg not in wsgi_server.error_log.ignored_msgs:
-                raise AssertionError(
-                    'Found error in the error log: '
-                    "message = '{}', level = '{}'\n"
-                    '{}'.format(c.msg, c.level, c.traceback),
-                )
+        if c.level <= logging.WARNING:
+            continue
+
+        assert c.msg in wsgi_server.error_log.ignored_msgs, (
+            'Found error in the error log: '
+            "message = '{c.msg!s}', level = '{c.level!s}'\n"
+            '{c.traceback!s}'.format(**locals()),
+        )
 
 
 @pytest.fixture
@@ -984,7 +985,9 @@ def test_Content_Length_out(
 
     assert actual_status == expected_resp_status
     assert actual_resp_body == expected_resp_body
+
     conn.close()
+
     # the server logs the exception that we had verified from the
     # client perspective. Tell the error_log verification that
     # it can ignore that message.
@@ -1043,9 +1046,10 @@ def test_No_CRLF(test_client, invalid_terminator):
     conn.close()
 
 
-def test_invalid_selected_connection(test_client, monkeypatch):  # noqa: D200,D205,D400,E501
-    """Test the error handling segment of
-    ``cheroot.connections.ConnectionManager.get_conn``.
+def test_invalid_selected_connection(test_client, monkeypatch):
+    """Test the error handling segment of HTTP connection selection.
+
+    See ``cheroot.connections.ConnectionManager.get_conn``.
     """
     class FaultySelect:
         def __init__(self, original_select):
@@ -1057,8 +1061,8 @@ def __call__(self, timeout):
             if self.request_served:
                 self.os_error_triggered = True
                 raise OSError('Error while selecting the client socket.')
-            else:
-                return self.original_select(timeout)
+
+            return self.original_select(timeout)
 
     class FaultyGetMap:
         def __init__(self, original_get_map):

From c94eaabb400f885a93aa9bf79a7153a6d8581d41 Mon Sep 17 00:00:00 2001
From: Liam Staskawicz <liam@stask.net>
Date: Tue, 18 Aug 2020 09:40:27 -0700
Subject: [PATCH 13/16] server: make connections 'private'

---
 cheroot/connections.py        |  4 ++--
 cheroot/server.py             | 27 +++++++++++++++++++--------
 cheroot/workers/threadpool.py |  2 +-
 3 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/cheroot/connections.py b/cheroot/connections.py
index 359cba1850..b230307cb2 100644
--- a/cheroot/connections.py
+++ b/cheroot/connections.py
@@ -276,8 +276,8 @@ def close(self):
         self._selector.close()
 
     @property
-    def _num_connections(self):  # noqa: D401
-        """The current number of connections.
+    def _num_connections(self):
+        """Return the current number of connections.
 
         Includes any in the readable list or registered with the selector,
         minus one for the server socket, which is always registered
diff --git a/cheroot/server.py b/cheroot/server.py
index 42decc2a7c..5405b2c746 100644
--- a/cheroot/server.py
+++ b/cheroot/server.py
@@ -1162,10 +1162,7 @@ def send_headers(self):  # noqa: C901  # FIXME
         # Override the decision to not close the connection if the connection
         # manager doesn't have space for it.
         if not self.close_connection:
-            can_keep = (
-                self.server.ready
-                and self.server.connections.can_add_keepalive_connection
-            )
+            can_keep = self.server.can_add_keepalive_connection
             self.close_connection = not can_keep
 
         if b'connection' not in hkeys:
@@ -1784,7 +1781,8 @@ def prepare(self):  # noqa: C901  # FIXME
         self.socket.settimeout(1)
         self.socket.listen(self.request_queue_size)
 
-        self.connections = connections.ConnectionManager(self)
+        # must not be accessed once stop() has been called
+        self._connections = connections.ConnectionManager(self)
 
         # Create worker threads
         self.requests.start()
@@ -1832,6 +1830,19 @@ def _run_in_thread(self):
         finally:
             self.stop()
 
+    @property
+    def can_add_keepalive_connection(self):
+        """Flag whether it is allowed to add a new keep-alive connection."""
+        return self.ready and self._connections.can_add_keepalive_connection
+
+    def put_conn(self, conn):
+        """Put an idle connection back into the ConnectionManager."""
+        if self.ready:
+            self._connections.put(conn)
+        else:
+            # server is shutting down, just close it
+            conn.close()
+
     def error_log(self, msg='', level=20, traceback=False):
         """Write error message to log.
 
@@ -2024,7 +2035,7 @@ def resolve_real_bind_addr(socket_):
 
     def tick(self):
         """Accept a new connection and put it on the Queue."""
-        conn = self.connections.get_conn()
+        conn = self._connections.get_conn()
         if conn:
             try:
                 self.requests.put(conn)
@@ -2032,7 +2043,7 @@ def tick(self):
                 # Just drop the conn. TODO: write 503 back?
                 conn.close()
 
-        self.connections.expire()
+        self._connections.expire()
 
     @property
     def interrupt(self):
@@ -2100,7 +2111,7 @@ def stop(self):  # noqa: C901  # FIXME
                 sock.close()
             self.socket = None
 
-        self.connections.close()
+        self._connections.close()
         self.requests.stop(self.shutdown_timeout)
 
 
diff --git a/cheroot/workers/threadpool.py b/cheroot/workers/threadpool.py
index b9987d9de3..6e6c721d90 100644
--- a/cheroot/workers/threadpool.py
+++ b/cheroot/workers/threadpool.py
@@ -120,7 +120,7 @@ def run(self):
                     keep_conn_open = conn.communicate()
                 finally:
                     if keep_conn_open:
-                        self.server.connections.put(conn)
+                        self.server.put_conn(conn)
                     else:
                         conn.close()
                     if is_stats_enabled:

From 965059379b246b3f2aef9c37648cf2140e35810b Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 16:44:52 -0500
Subject: [PATCH 14/16] Implement PR review feedback.

Mainly moving class definition to the top level and adding more docstring.

The most notable thing is the replacement of the fixture `testing_server`
to `raw_testing_server` and move the error_log patching in a new one
that will include that functionality using the old name `testing_server`.
---
 cheroot/test/test_conn.py | 126 +++++++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 49 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 26b3dda512..80785cced7 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -111,8 +111,32 @@ def _munge(string):
     }
 
 
+class ErrorLogMonitor:
+    """Mock class to access the server error_log calls made by the server."""
+
+    FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
+
+    def __init__(self):
+        """Initialize the server error log monitor/interceptor.
+
+        If you need to ignore a particular error message use the property
+        ``ignored_msgs` by appending to the list the expected error messages.
+        """
+        self.calls = []
+        # to be used the the teardown validation
+        self.ignored_msgs = []
+
+    def __call__(self, msg='', level=logging.INFO, traceback=False):
+        """Intercept the call to the server error_log method."""
+        if traceback:
+            tblines = traceback_.format_exc()
+        else:
+            tblines = ''
+        self.calls.append(ErrorLogMonitor.FuncCall(msg, level, tblines))
+
+
 @pytest.fixture
-def testing_server(wsgi_server_client, monkeypatch):
+def raw_testing_server(wsgi_server_client):
     """Attach a WSGI app to the given server and preconfigure it."""
     app = Controller()
 
@@ -126,35 +150,29 @@ def _timeout(req, resp):
     wsgi_server.server_client = wsgi_server_client
     wsgi_server.keep_alive_conn_limit = 2
 
-    # patch the error_log calls of the server instance
-    class ErrorLog:  # noqa: D200
-        """
-        Mock class to access the server error_log calls made by the server.
-        """
+    return wsgi_server
+
 
-        FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
+@pytest.fixture
+def testing_server(raw_testing_server, monkeypatch):
+    """Modify the "raw" base server to monitor the error_log messages.
 
-        def __init__(self):
-            self.calls = []
-            # to be used the the teardown validation
-            self.ignored_msgs = []
+    If you need to ignore a particular error message use the property
+    ``testing_server.error_log.ignored_msgs`` by appending to the list
+    the expected error messages.
+    """
+    # patch the error_log calls of the server instance
+    monkeypatch.setattr(raw_testing_server, 'error_log', ErrorLogMonitor())
 
-        def __call__(self, msg='', level=logging.INFO, traceback=False):
-            if traceback:
-                tblines = traceback_.format_exc()
-            else:
-                tblines = ''
-            self.calls.append(ErrorLog.FuncCall(msg, level, tblines))
+    yield raw_testing_server
 
-    monkeypatch.setattr(wsgi_server, 'error_log', ErrorLog())
-    yield wsgi_server
     # Teardown verification, in case that the server logged an
     # error that wasn't notified to the client or we just made a mistake.
-    for c in wsgi_server.error_log.calls:
+    for c in raw_testing_server.error_log.calls:
         if c.level <= logging.WARNING:
             continue
 
-        assert c.msg in wsgi_server.error_log.ignored_msgs, (
+        assert c.msg in raw_testing_server.error_log.ignored_msgs, (
             'Found error in the error log: '
             "message = '{c.msg!s}', level = '{c.level!s}'\n"
             '{c.traceback!s}'.format(**locals()),
@@ -1046,39 +1064,49 @@ def test_No_CRLF(test_client, invalid_terminator):
     conn.close()
 
 
+class FaultySelect:
+    """Mock class to insert errors in the selector.select method."""
+
+    def __init__(self, original_select):
+        """Initilize helper class to wrap the selector.select method."""
+        self.original_select = original_select
+        self.request_served = False
+        self.os_error_triggered = False
+
+    def __call__(self, timeout):
+        """Intercept the calls to selector.select."""
+        if self.request_served:
+            self.os_error_triggered = True
+            raise OSError('Error while selecting the client socket.')
+
+        return self.original_select(timeout)
+
+
+class FaultyGetMap:
+    """Mock class to insert errors in the selector.get_map method."""
+
+    def __init__(self, original_get_map):
+        """Initilize helper class to wrap the selector.get_map method."""
+        self.original_get_map = original_get_map
+        self.sabotage_conn = False
+        self.socket_closed = False
+
+    def __call__(self):
+        """Intercept the calls to selector.get_map."""
+        if self.sabotage_conn:
+            for _, (*_, conn) in self.original_get_map().items():
+                if isinstance(conn, cheroot.server.HTTPConnection):
+                    # close the socket to cause OSError
+                    conn.close()
+                    self.socket_closed = True
+        return self.original_get_map()
+
+
 def test_invalid_selected_connection(test_client, monkeypatch):
     """Test the error handling segment of HTTP connection selection.
 
     See ``cheroot.connections.ConnectionManager.get_conn``.
     """
-    class FaultySelect:
-        def __init__(self, original_select):
-            self.original_select = original_select
-            self.request_served = False
-            self.os_error_triggered = False
-
-        def __call__(self, timeout):
-            if self.request_served:
-                self.os_error_triggered = True
-                raise OSError('Error while selecting the client socket.')
-
-            return self.original_select(timeout)
-
-    class FaultyGetMap:
-        def __init__(self, original_get_map):
-            self.original_get_map = original_get_map
-            self.sabotage_conn = False
-            self.socket_closed = False
-
-        def __call__(self):
-            if self.sabotage_conn:
-                for _, (*_, conn) in self.original_get_map().items():
-                    if isinstance(conn, cheroot.server.HTTPConnection):
-                        # close the socket to cause OSError
-                        conn.close()
-                        self.socket_closed = True
-            return self.original_get_map()
-
     # patch the select method
     faux_select = FaultySelect(
         test_client.server_instance.connections._selector.select,

From 4f044fe746d91d4a8cd9515f250c4384813f710f Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 16:54:15 -0500
Subject: [PATCH 15/16] Use the same name for the namedtuple used in the
 ErrorLogMonitor class.

---
 cheroot/test/test_conn.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 80785cced7..522a6a8d19 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -114,7 +114,7 @@ def _munge(string):
 class ErrorLogMonitor:
     """Mock class to access the server error_log calls made by the server."""
 
-    FuncCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
+    ErrorLogCall = namedtuple('ErrorLogCall', ['msg', 'level', 'traceback'])
 
     def __init__(self):
         """Initialize the server error log monitor/interceptor.
@@ -132,7 +132,7 @@ def __call__(self, msg='', level=logging.INFO, traceback=False):
             tblines = traceback_.format_exc()
         else:
             tblines = ''
-        self.calls.append(ErrorLogMonitor.FuncCall(msg, level, tblines))
+        self.calls.append(ErrorLogMonitor.ErrorLogCall(msg, level, tblines))
 
 
 @pytest.fixture

From a4cfe029f00d517682c51142244d855fe4d23593 Mon Sep 17 00:00:00 2001
From: Joel Rivera <rivera@joel.mx>
Date: Sun, 23 Aug 2020 17:03:30 -0500
Subject: [PATCH 16/16] Small refactor to conform to review comments.

---
 cheroot/test/test_conn.py | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/cheroot/test/test_conn.py b/cheroot/test/test_conn.py
index 522a6a8d19..8c16b6fbe4 100644
--- a/cheroot/test/test_conn.py
+++ b/cheroot/test/test_conn.py
@@ -174,8 +174,8 @@ def testing_server(raw_testing_server, monkeypatch):
 
         assert c.msg in raw_testing_server.error_log.ignored_msgs, (
             'Found error in the error log: '
-            "message = '{c.msg!s}', level = '{c.level!s}'\n"
-            '{c.traceback!s}'.format(**locals()),
+            "message = '{c.msg}', level = '{c.level}'\n"
+            '{c.traceback}'.format(**locals()),
         )
 
 
@@ -1093,12 +1093,16 @@ def __init__(self, original_get_map):
 
     def __call__(self):
         """Intercept the calls to selector.get_map."""
-        if self.sabotage_conn:
-            for _, (*_, conn) in self.original_get_map().items():
-                if isinstance(conn, cheroot.server.HTTPConnection):
-                    # close the socket to cause OSError
-                    conn.close()
-                    self.socket_closed = True
+        sabotage_targets = (
+            conn for _, (*_, conn) in self.original_get_map().items()
+            if isinstance(conn, cheroot.server.HTTPConnection)
+        ) if self.sabotage_conn else ()
+
+        for conn in sabotage_targets:
+            # close the socket to cause OSError
+            conn.close()
+            self.socket_closed = True
+
         return self.original_get_map()