Opened 15 years ago

Closed 15 years ago

#1965 defect closed fixed (fixed)

AMP "LiveFire" unit test setup contains race condition

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

LiveFireBase has a setUp that looks like this:

    def setUp(self):
        from twisted.internet import reactor
        self.serverFactory = protocol.ServerFactory()
        self.serverFactory.protocol = self.serverProto
        self.clientFactory = protocol.ClientFactory()
        self.clientFactory.protocol = self.clientProto
        self.clientFactory.onMade = defer.Deferred()
        self.serverPort = reactor.listenTCP(0, self.serverFactory)
        self.clientConn = reactor.connectTCP(
            '127.0.0.1', self.serverPort.getHost().port,
            self.clientFactory)
        def getProtos(rlst):
            self.cli = self.clientFactory.theProto
            self.svr = self.serverFactory.theProto
        return self.clientFactory.onMade.addCallback(getProtos)

This is incorrect: it assumes that the server will have had buildProtocol called by the time the client's connectionMade callback runs. The code needs to wait on a Deferred for each side of the connection.

I haven't looked at the rest of the tests so I don't know of similar races are present elsewhere.

Technically this is a test suite regression and the AMP branch should be reverted, but the problem appears to be purely in the tests and the failure is only triggered on one reactor. Assuming this is fixed ASAP it's probably okay not to revert the branch.

Someone should go over the rest of the tests AMP added, though, and make sure there are no other similar problems.

Change History (6)

comment:1 Changed 15 years ago by Glyph

Starting work on this now in patience-1965.

FWIW, the live fire tests are the only ones that do anything with actual sockets, so unless you find another such race condition in tearDown it is unlikely that the other tests are going to cause problems.

comment:2 Changed 15 years ago by Glyph

Keywords: review added
Owner: changed from Glyph to Jean-Paul Calderone

OK, that branch is ready for review. Should be a quickie for anyone interested, but assigning to exarkun as the reporter.

comment:3 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Glyph

The tearDown on the same class also incorrectly assumes stopListening always returns a Deferred. I think the use of a DeferredList like that is also somewhat sketchy. It needs to block the ConnectionLost/Done errback, but it is also consuming and discarding all other errors without checking their types. I doubt this is masking anything or will ever mask anything, but it's somewhat sloppy.

comment:4 Changed 15 years ago by Jean-Paul Calderone

To clarify, fix the tearDown Deferred handling bug and then merge it.

comment:5 Changed 15 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [17778]) Fix race conditions and test ugliness in test_amp.

Author: glyph

Reviewer: exarkun

Fixes #1965

There were 2 bugs in test_amp's setUp and tearDown; this fixes them. First, the QT reactor and win32event reactor buildbots were failing due to a race condition between the client and the server's connectionMade methods, and second, 'stopListening' doesn't always return a Deferred, so the tests now deal with that.

comment:6 Changed 11 years ago by <automation>

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