I will try to look at this when I'm at the core sprint, Sept 23-27.

@paravoid The above code doesn't work; it crashes on line 6. I guess you didn't actually run it before filing the bug? Also I don't know what nc does with these parameters, could you please add a client that triggers the issue so I can understand, or at least explain what this nc invocation does?

Finally, your code differs (apart from the crash) from the original code in several ways:

• The handler never writes back to the writer
• It has a try/except around serve_forever()

Which of these is significant?

@gvanrossum Thanks for taking a look! I did run the code but tried simplifying it after the fact (it's all in the GitHub history). Fixed now, it was a s/line/data/. Sorry about that!

nc just connects and waits for input. There is no need to do anything on the socket. Instead of nc you can also do: python3 -c "import socket, time; s = socket.socket(socket.AF_INET, socket.SOCK_STREAM); s.connect(('localhost', 8888)); time.sleep(100)" in another window.

Finally, your code differs (apart from the crash) from the original code in several ways:
* The handler never writes back to the writer
* It has a try/except around serve_forever()

Which of these is significant?

None of the two are significant. The write has been removed for simplicity (it doesn't matter). The try/except is there to make it easier to see that error handling does work when a client is not connected.

My first reaction, after putting a bunch of print calls in the code you quoted in Server.close() in base_events.py, is that this was done intentionally (but without thinking through all the consequences).

The cure would then to be to put a timeout in each handler, e.g. async with asyncio.timeout(5) around the await reader.read(100). Then the behavior will be:

• If a request comes in and the client hangs, and you don't interrupt the server, after 5 seconds the handler will terminate with a TimeoutError, which will be logged (and the server will close the connection), and the server will continue serving. (And the client's connection will be closed, although if you use a hard time.sleep(100) it will not realize this.)
• If a request comes in, the client hangs, and you hit ^C before the handler times out, the handler will continue to wait until its 5 seconds are up, and then raise TimeoutError as before. At that point the server's wait_closed() detects that the last handler is done and exits, continuing the path taken by the ^C.

You could also catch the TimeoutError and continue to close the connection (so you don't get a traceback logged). However it looks that in this case you will hang in the handler's await writer.wait_closed() until the client cooperates (which might be never) so you still have the same problem. It seems that a secondary bug is that that await doesn't honor timeouts -- this is something we might be able to fix separately.

In any case, this gives a workaround for 3.12 and 3.13 (the latter isn't out yet but has been in feature freeze for months) but it's far from perfect. I'm not sure what to do to make the experience better without adding timeouts to every handler. Maybe the solution is just to send CancelledError into every handler that's still active when the server is closed? That's close to what I assumed was happening before 3.12 -- the server process responds immediately (?) to ^C, so the handlers are effectively abandoned.

What do you think, @paravoid ?

Here's a diff that appears to fix your issue and break no tests (though it clearly needs a new test that will demonstrate it changed something).

index ffcc0174e1e..a42c4ec0037 100644
--- a/Lib/asyncio/base_events.py
+++ b/Lib/asyncio/base_events.py
@@ -381,6 +381,7 @@ async def serve_forever(self):
         except exceptions.CancelledError:
             try:
                 self.close()
+                self.close_clients()
                 await self.wait_closed()
             finally:
                 raise

Here's a diff that appears to fix your issue and break no tests (though it clearly needs a new test that will demonstrate it changed something).

Ah! That's neat. I originally discovered this in 3.12 and only later reproduced it in 3.13, so I hadn't noticed the addition of close_clients(). This fix seems so simple in retrospect! I tried it out and it seems to work as intended with my limited testing.

In light of this fix, it may be entirely moot now, but with regards to your previous message: adding arbitrarily-valued timeouts doesn't strike me as a great solution. The value may be too short for legitimate use cases, forcing users to have to reimplement retries in a loop to read() again etc. It'll be even more of a nightmare with readline(). That's going to be a lot of tricky boilerplate code, to address pitfalls similar to the ones that PEP 475 (Retry system calls failing with EINTR) tried to address IMHO. At the same time, the timeout value may also be too long, and a noticeable lag for shutdowns.

I will have to ponder whether it's okay to backport this to 3.12 and 3.13. I can't decide whether it's a new feature that might break code that worked before in 3.12 or 3.13, or just a bug fix (which would qualify for backporting). To deal with the cancellation, handlers that want to send a final goodbye to their clients will now have to catch CancelledError. Then again in 3.12/3.13 they currently have no way to tell that the server is being closed, so, yeah, I'm thinking of it more and more as a bugfix that more or less restores the old (3.11 and earlier) behavior.

I have to come up with a test case too. :-(

Thanks so much for pushing a fix! My (non-strong) opinion is that this deserves to be fixed in a minor release, both because it's a regression compared to 3.11, and because it's hard to workaround in downstream code, at least without reimplementing some of stdlib's internals. (For 3.12 it's even worse, because close_clients() doesn't even exist there.)

I will try to find time to complete the PR. I will have to ask the 3.12 and 3.13 release managers for their opinion on the backports.

It seems like we lost our momentum... I see there is an unmerged PR to fix this -- any way I can help to move this forward? Thanks again!

Hi @gvanrossum, hope you have been well!

I have to come up with a test case too. :-(

Here's a crude test that will raise a TimeoutError without Server.close_clients() and will pass with your fix:

Edit: Updated to include @pskopnik suggestion!

test_asyncio.test_server.TestServer2

class TestServer2(unittest.IsolatedAsyncioTestCase):

    # ... existing tests

    async def test_close_with_hanging_client(self):
        # Synchronize server cancellation only after the socket connection is
        # accepted and this event is set
        conn_event = asyncio.Event()
        class Proto(asyncio.Protocol):
            def connection_made(self, transport):
                conn_event.set()

        loop = asyncio.get_running_loop()
        srv = await loop.create_server(Proto, socket_helper.HOSTv4, 0)

        # Start the server
        serve_forever_task = asyncio.create_task(srv.serve_forever())
        await asyncio.sleep(0)

        # Create a connection to server
        addr = srv.sockets[0].getsockname()
        sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
        sock.connect(addr)
        self.addCleanup(sock.close)

        # Send a CancelledError into the server to emulate a Ctrl+C
        # KeyboardInterrupt whilst the server is handling a hanging client
        await conn_event.wait()
        serve_forever_task.cancel()

        # Ensure the client is closed within a timeout
        async with asyncio.timeout(0.5):
            await srv.wait_closed()

        self.assertFalse(srv.is_serving())

@ordinary-jamie, thanks for putting that together :)
Small change suggestion: Store the serve_forever() task in a variable and cancel the task instead of setting the exception on the internal future. Now you are purely testing the public API!