gh-113538: Allow client connections to be closed

[CendioOssman]

Give applications the option of more forcefully terminating client connections for asyncio servers. Useful when terminating a service and there is limited time to wait for clients to finish up their work.

• Issue: Cannot cleanly shut down an asyncio based server #113538

---

📚 Documentation preview 📚: https://cpython-previews--114432.org.readthedocs.build/

[CendioOssman]

PR includes unit tests, but I also used this more complete example to test and showcase the functionality:

#!/usr/bin/python3importasyncioimportsocket# Well behaved or spammy protocol?FLOOD=TrueclassEchoServerProtocol(asyncio.Protocol): defconnection_made(self, transport): peername=transport.get_extra_info('peername') print('Connection from{}'.format(peername)) self.transport=transportself._paused=Falseself._write() defpause_writing(self): self._paused=Truedefresume_writing(self): self._paused=Falseself._write() def_write(self): ifnotFLOOD: returnifself._paused: returnself.transport.write(b'a'*65536) asyncio.get_running_loop().call_soon(self._write) defdata_received(self, data): message=data.decode() print('Data received:{!r}'.format(message)) defstop(): loop=asyncio.get_running_loop() loop.stop() client=Nonedefserver_up(fut): globalserverserver=fut.result() print('Server running') globalclientclient=socket.create_connection(('127.0.0.1', 8888)) loop=asyncio.new_event_loop() asyncio.set_event_loop(loop) fut=asyncio.ensure_future(loop.create_server( lambda: EchoServerProtocol(), '127.0.0.1', 8888)) fut.add_done_callback(server_up) # Simulate terminationloop.call_later(2.5, stop) try: loop.run_forever() finally: print('Closing...') server.close() try: loop.run_until_complete(asyncio.wait_for(server.wait_closed(), 0.1)) exceptTimeoutError: print('Timeout waiting for clients. Attempting close...') server.close_clients() try: loop.run_until_complete(asyncio.wait_for(server.wait_closed(), 0.1)) exceptTimeoutError: print('Timeout waiting for client close. Attempting abort...') server.abort_clients() loop.run_until_complete(asyncio.wait_for(server.wait_closed(), 0.1)) loop.close() ifclientisnotNone: client.close() print('Closed')

[gvanrossum]

This looks great, but I have one reservation. The server now keeps the transports alive, through the set of clients. Maybe it would be better if that was a weak set, so that if somehow a transport was freed without going through the usual close routine, the server doesn't hold onto it forever? Thoughts?

[CendioOssman]

I figured I'd start things simple, so no weak references unless there is a practical issue that requires them.

So, are there any scenarios where this is relevant?

The transport detaches from the server in the same place where it closes the server. That should mean that this is something that must be done, as otherwise the destructor will complain. So I don't think well-written code should have an issue.

Might there be an issue with badly written code, then? The primary case would be if the destructor check fails to trigger. Which I guess is the case with my suggestion. It is difficult to trigger, though. Streams are a bit buggy, so abandoning them doesn't always result in a cleanup (#114914), and transports are referenced by the event loop until you tell them to pause reading.

But difficult isn't impossible, so this case breaks when applying this PR:

#!/usr/bin/python3importasyncioimportsocketclassEchoServerProtocol(asyncio.Protocol): defconnection_made(self, transport): peername=transport.get_extra_info('peername') print('Connection from{}'.format(peername)) self.transport=transportself.transport.pause_reading() defdata_received(self, data): message=data.decode() print('Data received:{!r}'.format(message)) client=Nonedefserver_up(fut): globalserverserver=fut.result() print('Server running') globalclientclient=socket.create_connection(('127.0.0.1', 8888)) loop=asyncio.new_event_loop() asyncio.set_event_loop(loop) fut=asyncio.ensure_future(loop.create_server( lambda: EchoServerProtocol(), '127.0.0.1', 8888)) fut.add_done_callback(server_up) # Simulate terminationloop.call_later(2.5, loop.stop) try: loop.run_forever() importgcgc.collect() finally: server.close() loop.close() ifclientisnotNone: client.close()

This case creates a transport, pauses it, and then fails to maintain any other reference to it. Without this PR it gets collected when gc.collect() is called (or earlier). With this PR, it only gets collected at the end.

So a weak reference does indeed seem appropriate.

[gvanrossum]

Sorry for the delay -- this just made it to the top of my review queue. I hope to get to this soon!

[gvanrossum]

Not bad, some thoughts.

[gvanrossum]

Suggested change

	# Weak references so abandoned transports can be detected
	# Weak references so abandoned transports can be ignored

[CendioOssman]

The wording here was intentional. Weak references is to my knowledge the only way to detect abandoned objects. But it's not this code that does that detection, so I can understand the confusion. How about:

Weak references so we don't break Transport's ability to detect abandoned transports

?

[gvanrossum]

Ah, you're thinking from the POV of the transport, whose __del__ must be called to "detect" (i.e., warn about) that it was abandoned. I was thinking from the POV of the loop in close_clients(), where we want to ignore (not encounter) transports that have been closed already.

I'll make it your choice which wording to use.

[gvanrossum]

Yeah, short sleeps are a nuisance in asyncio tests. Usually they can be replaced by a small number of sleep(0) calls though -- usually 1, rarely 2 or 3. sleep(0) is special and guarantees we go through the event loop exactly once.

[CendioOssman]

A bunch of sleep(0) felt even more arbitrary. :/

What's your suggestion here? Keep it as is? Or something else?

[gvanrossum]

A bunch of sleep(0) wastes less time (the asyncio tests are already too slow), and doesn't risk the test becoming flaky due to timing out on slow platforms (a problem we've struggled with). The right number of sleep(0) call takes up no more time than needed, and lets the machinery go through its motions in a deterministic matter. So I recommend sleep(0).

[gvanrossum]

Eh, what's going on with this one? Is it the same issue that can be fixed with a small number of sleep(0) calls, or different?

[CendioOssman]

I don't know, to be honest. Might be a kernel or libc issue where it shuffles buffers around and/or allocates more space.

Without it, I cannot reliably get both the server and client to a state where buffers are full. Which is needed for the test to check the right thing.

[gvanrossum]

IMO this PR is not finished until you get to the bottom of that. If you really need then to reach a specific state and there's no deterministic way to get there, consider manipulating internal APIs.

[CendioOssman]

This was a pain, but I think I got it fixed. The core problem is that the kernel dynamically increases the socket send buffer size. And it takes about 20-30 ms to do so.

I think I've worked around that by specifying an explicit buffer size. That should turn off the dynamic resizing, if I remember things correctly.

[gvanrossum]

This one looks similar to the first sleep.

[gvanrossum]

Ah, you're thinking from the POV of the transport, whose __del__ must be called to "detect" (i.e., warn about) that it was abandoned. I was thinking from the POV of the loop in close_clients(), where we want to ignore (not encounter) transports that have been closed already.

I'll make it your choice which wording to use.

[gvanrossum]

A bunch of sleep(0) wastes less time (the asyncio tests are already too slow), and doesn't risk the test becoming flaky due to timing out on slow platforms (a problem we've struggled with). The right number of sleep(0) call takes up no more time than needed, and lets the machinery go through its motions in a deterministic matter. So I recommend sleep(0).

[gvanrossum]

IMO this PR is not finished until you get to the bottom of that. If you really need then to reach a specific state and there's no deterministic way to get there, consider manipulating internal APIs.

[gvanrossum]

IMO you don't need this comment either (it was explaining the condition).

[gvanrossum]

Thanks for fixing all that. I have one remaining concern only.

[gvanrossum]

Can we put an upper bound on these while loops and fail the test when they loop too many times? (Possibly a for loop with a break is the best idiom to do this.)

I worry that something in the network stack might cause one or the other to loop forever, and I'd rather not waste the CPU time in CI over this.

How many iterations do you expect? Is it deterministic regardless of platform?

[CendioOssman]

It's a bit sensitive to platform behaviour, but I believe I can get something that dynamically adapts

[gvanrossum]

LG, but can you fix the merge conflict? The tests won't run until that's fixed.

[CendioOssman]

Sure. How do you prefer to handle that? A rebase against current main?

[gvanrossum]

No rebase please! Please merge the updated main branch into your branch, fix the conflicts as part of the merge, and then commit and push (not push -f). The extra commits will be squashed when I merge back into main.

[gvanrossum]

Thanks! LGTM. I'll merge.

[mhsmith]

It looks like one of the new tests isn't always passing: #116423 (comment)

[gvanrossum]

It looks like one of the new tests isn't always passing: #116423 (comment)

Thanks, I've created a new PR on the same issue that will revert it. Sorry for the inconvenience.

[gvanrossum]

The PR has been reverted.

@CendioOssman could you look into the test failures? I probably should have reviewed the test more carefully, sorry.

[CendioOssman]

Of course. And once they are fixed, how do you want to proceed? Submit a new PR using the existing branch?

[gvanrossum]

Of course. And once they are fixed, how do you want to proceed? Submit a new PR using the existing branch?

That sounds fine to me. If you can't get it to work, just rename the branch locally and push that.