Are the unused_tcp_port fixtures really useful for trio?

[njsmith]

I'm not sure either way, so here's an issue to discuss it.

The reason why they might not be: here's a little trio test of a simple server:

# Placeholder for some more interesting server
async def echo(stream):
    while True:
        data = await stream.receive_some(4096)
        if data:
            await stream.send_all(data)
        else:
            await stream.aclose()
            return

async def echo_server(port, *, task_status=trio.TASK_STATUS_IGNORED):
    await trio.serve_tcp(echo, port, task_status=task_status)

# Okay here's the test for our server
# This version is BROKEN
async def test_echo_server(unused_tcp_port):
    async with trio.open_nursery() as nursery:
        nursery.start_soon(echo_server, unused_tcp_port)
        stream = await trio.open_tcp_stream("localhost", unused_tcp_port)
        await stream.send_all(b"x")
        assert await stream.receive_some(1) == b"x"
        # Test finished; shut down the server
        nursery.cancel_scope.cancel()

This is broken... well, in two ways, actually.

The very minor way it's broken is intrinsic to the use of unused_tcp_port: there's always a race condition between when unused_tcp_port checks to find the open port, and when we actually use it -- someone might open the port in the mean time. This is rarely a problem, but it does bug me a bit.

There's also a much more important race condition: because we spawn the server in the background, and then immediately attempt to connect to it, we might end up connecting before the server is actually started up and has bound its port. In my experience this is a very common problem with other frameworks, and often requires ugly hacks like sticking a retry loop around the connect call.

Here's a better way to write this same test:

async def test_echo_server():
    async with trio.open_nursery() as nursery:
        # This uses nursery.start, which waits for the server to be started up
        listeners = await nursery.start(echo_server, 0)
        stream = await trio.testing.open_stream_to_socket_listener(listeners[0])
        await stream.send_all(b"x")
        assert await stream.receive_some(1) == b"x"
        # Test finished; shut down the server
        nursery.cancel_scope.cancel()

We changed two things here: First, we directly told our server to bind to port 0, i.e., to pick a random unused port. In the old way of doing things, this would be a problem, because we wouldn't have any way to know what port it ended up picking. And second, we used nursery.start instead of nursery.start_soon.

Using nursery.start fixes the big important race condition, because it doesn't return until after the background task (in this case our echo server) has fully finished initializing itself. So by the time we make a client connection, we know the server is ready to accept clients.

But nursery.start also fixes our problem with knowing which port was picked, because it passes back a value after the server was initialized – in this case the value comes from trio.serve_tcp, which passes back the SocketListeners that it opened. And then can use `trio.testing.open_stream_to_socket_listener to connect to one of these listeners -- it automatically introspects it to figure out what port it's using, and makes a connection.

Summing up: this way of solving our problem uses 1 less feature, while solving 2 problems, in the same number of lines of code.

So given that trio has these features, I'm wondering whether unused_tcp_port is something we want to include or not. It's potentially still useful in some cases, I guess, and if nothing else then people who are expecting it might be grumpy if they don't find it. But OTOH maybe including it will just encourage people to do things the old kinda-broken way. As a general rule, if there are two ways of doing something and one of them works better than the other, trio tries to only include the one that works better :-)

[touilleMan]

There's also a much more important race condition: because we spawn the server in the background, and then immediately attempt to connect to it, we might end up connecting before the server is actually started up and has bound its port. In my experience this is a very common problem with other frameworks, and often requires ugly hacks like sticking a retry loop around the connect call.

I can very much relate ! That's precisely what I was doing with trio at the very beginning (using a trio.Event to say when the server was ready to receive connections), switching to the listener system was a very big improvement ("easy to do the right thing" philosophy)

However I still make use of the unused_tcp_port for two things:

1. Getting a unused port and keeping it this way !
   This is useful to test how a client would react if the server it should connect to is not available

2. Configuring a class that does connection to the server internally
   In my usecase I have a client A and two servers (so each one started by a trio.serve_tcp) B&C.
   A connects to B which in turn connects to C.
   When I want to test the interface between A and B, I must provide the port where C is running as configuration when starting B. Using unused_tcp_port means I don't have to hack/patch B connection to C code to make it uses trio.testing.open_stream_to_socket_listener.

But OTOH maybe including it will just encourage people to do things the old kinda-broken way

I pretty much agree (I a living example of this !), I think we should remove this unused_tcp_port fixture (if user really need it, it's just a 5 liner copy/paste from stackoverflow) and improve the listener object to easily expose the tcp port (this way usecases 2) is addressed and usecase 1) can be easily done by starting and stopping the server).

However this means a special case for trio.SocketListener when doing tcp, other solution is to let user do listeners[0].socket.getsockname()[1] to retrieve the port number himself, but it seems really too low level and complicated for such a common thing.

[njsmith]

Those are great use cases!

Getting a unused port and keeping it this way

I wonder if we should make a closed_port fixture or something like that? The most reliable way to make sure a port is unused is to bind a socket to it and then not call listen – that way you're not listening, but you're also guaranteed that no-one else can claim the port and start listening. BUT, MacOS has some really screwy behavior where if you try to connect to a port that has nothing bound, it fails instantly, while if you try to connect to a port that's bound but not listening, it waits 30 seconds or so before failing. So a closed_port fixture would want to the reliable thing on Linux/Windows (bind a port and don't listen), and the theoretically-slightly-less-reliable-but-it-actually-works thing on MacOS (pick an unused port but don't bind it).

Or maybe this is overthinking things :-)

improve the listener object to easily expose the tcp port

Yeah, we should definitely do this, for lots of reasons – it's actually got an issue open for it already: python-trio/trio#280

I guess this is the other advantage of removing unused_tcp_port, it forces us to think of more specific solutions to these problems :-).

All that said, I'm not really bothered by the unused tcp port pattern; the theoretical race condition bugs me a bit, but in practice like... every networking library test suite uses it and it works fine in practice. I mostly don't want people to be using the more awkward approaches just because they don't realize that trio has alternatives.

Thinking about it more, I'm realizing that part of what's bugging me here is just that the use of fixtures for this is kind of awkward. The port ends up with a long name, there's no way to configure behavior (e.g. right now it has a baked in assumption that you're always going to be using the port for IPv4, not IPv6, and for TCP, not UDP), etc. Maybe what we should do is add a trio.testing.unused_port function, so you do:

from trio.testing import unused_port

async def some_test():
    tcp_port = unused_port()
    udp_port = unused_port(type=trio.socket.SOCK_DGRAM)
    ...

And then the docs for this function can point out the alternatives?

Anyway, it does sound for like we agree that dropping the fixtures from pytest-trio is a good idea, at least for now, so I'll make a PR :-)