Opened 4 years ago

Last modified 4 years ago

#4502 defect new

test_tcp does not wait for connectionLost before assuming a connection is dead

Reported by: ghazel Owned by:
Priority: normal Milestone:
Component: core Keywords: test_tcp connectionLost
Cc: Branch:
Author: Launchpad Bug:

Description

This seems to mostly work out by accident, but it's possible that transport.loseConnection() or connector.disconnect() could take a few iterations to call connectionLost and remove the selectable, leaving the reactor dirty.

An example offender:

test_tcp.ListeningTestCase.test_serverRepr


def test_serverRepr(self):
    """
    Check that the repr string of the server transport get the good port
    number if the server listens on 0.
    """
    server = MyServerFactory()
    serverConnMade = server.protocolConnectionMade = defer.Deferred()
    port = reactor.listenTCP(0, server)
    self.addCleanup(port.stopListening)

above: here it only queues a clean up for the tcp.Port object, not any Protocols it may have created.

    client = MyClientFactory()
    clientConnMade = client.protocolConnectionMade = defer.Deferred()
    connector = reactor.connectTCP("127.0.0.1",
                                   port.getHost().port, client)
    self.addCleanup(connector.disconnect)

above: this only queues connector.disconnect, it does not wait for connectionLost to occur.

    def check((serverProto, clientProto)):
        portNumber = port.getHost().port
        self.assertEquals(
            repr(serverProto.transport),
            "<AccumulatingProtocol #0 on %s>" % (portNumber,))
        serverProto.transport.loseConnection()
        clientProto.transport.loseConnection()
    return defer.gatherResults([serverConnMade, clientConnMade]
        ).addCallback(check)

above: the test specifically only waits for connectionMade.

The pair of loseConnections in the "check" callback is redundant with the connector.disconnect queued earlier, neither of which is sufficient to wait for connection lost on both connections.

The tests currently get away with it since cleanup calls occur in reverse and port.stopListening happens to take at least as many iterations as connector.disconnect on a socket with no write buffer. The two extra reactor.iterate(0) calls in _Janitor._cleanPending mean that even without those port.stopListening iterations, the sockets might get cleaned up.

Change History (2)

comment:1 Changed 4 years ago by exarkun

As long as the extra iterate calls are there, we probably won't notice when problems like this are introduced. Perhaps it would be nice if there were a way to ask trial to skip these, or mark things they cause to be cleaned up as an error. Ideally this would be something that could be applied case by case, so that new tests could have the stricter behavior, and as old tests are fixed they could be converted to avoid future regressions.

As long as there's not a way to automatically be told about these problems, I'm not sure how valuable fixing them is, since this kind of problem is very easy to re-introduce.

comment:2 Changed 4 years ago by <automation>

  • Owner glyph deleted
Note: See TracTickets for help on using tickets.