◐ Shell
reader mode source ↗
Skip to content

gh-113538: Allow client connections to be closed#114432

Merged
gvanrossum merged 14 commits into
python:mainfrom
CendioOssman:server_close
Mar 11, 2024
Merged

gh-113538: Allow client connections to be closed#114432
gvanrossum merged 14 commits into
python:mainfrom
CendioOssman:server_close

Conversation

@CendioOssman

@CendioOssman CendioOssman commented Jan 22, 2024

Copy link
Copy Markdown
Contributor

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.


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

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.
@CendioOssman

Copy link
Copy Markdown
Contributor Author

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

#!/usr/bin/python3

import asyncio
import socket

# Well behaved or spammy protocol?
FLOOD = True

class EchoServerProtocol(asyncio.Protocol):
    def connection_made(self, transport):
        peername = transport.get_extra_info('peername')
        print('Connection from {}'.format(peername))
        self.transport = transport
        self._paused = False
        self._write()

    def pause_writing(self):
        self._paused = True

    def resume_writing(self):
        self._paused = False
        self._write()

    def _write(self):
        if not FLOOD:
            return
        if self._paused:
            return
        self.transport.write(b'a' * 65536)
        asyncio.get_running_loop().call_soon(self._write)

    def data_received(self, data):
        message = data.decode()
        print('Data received: {!r}'.format(message))

def stop():
    loop = asyncio.get_running_loop()
    loop.stop()

client = None

def server_up(fut):
    global server
    server = fut.result()
    print('Server running')
    global client
    client = 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 termination
loop.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))
    except TimeoutError:
        print('Timeout waiting for clients. Attempting close...')
        server.close_clients()
        try:
            loop.run_until_complete(asyncio.wait_for(server.wait_closed(), 0.1))
        except TimeoutError:
            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()
    if client is not None:
        client.close()
    print('Closed')

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

Copy link
Copy Markdown
Contributor Author

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/python3

import asyncio
import socket

class EchoServerProtocol(asyncio.Protocol):
    def connection_made(self, transport):
        peername = transport.get_extra_info('peername')
        print('Connection from {}'.format(peername))
        self.transport = transport
        self.transport.pause_reading()

    def data_received(self, data):
        message = data.decode()
        print('Data received: {!r}'.format(message))

client = None

def server_up(fut):
    global server
    server = fut.result()
    print('Server running')
    global client
    client = 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 termination
loop.call_later(2.5, loop.stop)

try:
    loop.run_forever()
    import gc
    gc.collect()
finally:
    server.close()
    loop.close()
    if client is not None:
        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.

We want to be able to detect if the application fails to keep track of
the transports, so we cannot keep them alive by using a hard reference.
The application might be waiting for all transports to close, so we need
to properly inform the server that this transport is done.
@gvanrossum

Copy link
Copy Markdown
Member

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

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Not bad, some thoughts.

One could be made clearar, and the other is probably superfluous.
Try to get the streams and the kernel in to a more deterministic state
by specifying fixed buffering limits.

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

No possibly infinite loop. Instead ask the system how much buffer space
it has and fill that.

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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

11 hidden items Load more…
@CendioOssman

Copy link
Copy Markdown
Contributor Author

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

@gvanrossum

Copy link
Copy Markdown
Member

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.

@CendioOssman CendioOssman requested a review from gvanrossum March 11, 2024 15:15

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

Thanks! LGTM. I'll merge.

@gvanrossum gvanrossum merged commit 1d0d49a into python:main Mar 11, 2024
@mhsmith

mhsmith commented Mar 11, 2024

Copy link
Copy Markdown
Member

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

gvanrossum added a commit to gvanrossum/cpython that referenced this pull request Mar 11, 2024
@gvanrossum

Copy link
Copy Markdown
Member

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 added a commit that referenced this pull request Mar 12, 2024
…#114432)" (#116632)

Revert "gh-113538: Add asycio.Server.{close,abort}_clients (#114432)"

Reason: The new test doesn't always pass:
#116423 (comment)

This reverts commit 1d0d49a.
@gvanrossum

Copy link
Copy Markdown
Member

The PR has been reverted.

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

@CendioOssman

Copy link
Copy Markdown
Contributor Author

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

@gvanrossum

gvanrossum commented Mar 12, 2024

Copy link
Copy Markdown
Member

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.

gvanrossum pushed a commit that referenced this pull request Mar 18, 2024
These 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.

This is a do-over with a test fix for gh-114432, which was reverted.
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
…on#116784)

These 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.

This is a do-over with a test fix for pythongh-114432, which was reverted.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
These 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.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…ort}_clients (python#114432)" (python#116632)

Revert "pythongh-113538: Add asycio.Server.{close,abort}_clients (python#114432)"

Reason: The new test doesn't always pass:
python#116423 (comment)

This reverts commit 1d0d49a.
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
…on#116784)

These 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.

This is a do-over with a test fix for pythongh-114432, which was reverted.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
These 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.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…ort}_clients (python#114432)" (python#116632)

Revert "pythongh-113538: Add asycio.Server.{close,abort}_clients (python#114432)"

Reason: The new test doesn't always pass:
python#116423 (comment)

This reverts commit 1d0d49a.
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…on#116784)

These 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.

This is a do-over with a test fix for pythongh-114432, which was reverted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants