Opened 10 years ago

Closed 6 years ago

#3037 defect closed fixed (fixed)

loseWriteConnection breaks loseConnection

Reported by: mwicked Owned by: Itamar Turner-Trauring
Priority: normal Milestone:
Component: core Keywords:
Cc: jesstess Branch: branches/select-halfclose-3037
branch-diff, diff-cov, branch-cov, buildbot
Author: itamarst

Description (last modified by Jean-Paul Calderone)

ListenerProtocol1 leaks socket fd's each time it answers a query, and connectionLost is never triggered. ListenerProtocol2 works as expected.

class ListenerProtocol1(Protocol):
    """ Protocol that knows how to deal with connections to our listener """

    implements(IHalfCloseableProtocol)

    def __init__(self, poller):
        self.poller = poller
        self.data = []

    def dataReceived(self, data):
        self.data.append(data)

    def readConnectionLost(self):
        query = "".join(self.data).lower()
        self.transport.loseWriteConnection()

    def writeConnectionLost(self):
        self.transport.write("YO")
        log.msg("finished responding to query")
        self.transport.loseWriteConnection()

    def connectionLost(self, reason):
        log.msg("finished responding to query")




class ListenerProtocol2(Protocol):
    """ Protocol that knows how to deal with connections to our listener """

    implements(IHalfCloseableProtocol)

    def __init__(self, poller):
        self.poller = poller
        self.data = []

    def dataReceived(self, data):
        self.data.append(data)

    def readConnectionLost(self):
        query = "".join(self.data).lower()
        self.transport.write("Yo")
        self.transport.loseConnection()

    def connectionLost(self, reason):
        log.msg("finished responding to query")

Attachments (1)

3037.patch (1.8 KB) - added by Zubin Mithra 7 years ago.
All the changes mentioned above have been made.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by Jean-Paul Calderone

Description: modified (diff)

Editing markup.

comment:2 Changed 10 years ago by mwicked

Sorry, there is a typo in the problem. In the ListenerProtocol1, the second call to loseWriteConnection should be loseConnection instead.

comment:3 Changed 7 years ago by Glyph

Owner: changed from Glyph to jknight

Reassigning since you are the guy who likes half-close.

comment:4 Changed 7 years ago by jknight

Keywords: easy added

Test case:

from twisted.internet import protocol, interfaces
from zope.interface import implements
class ListenerProtocol1(protocol.Protocol):
    """ Protocol that knows how to deal with connections to our listener """

    implements(interfaces.IHalfCloseableProtocol)

    def __init__(self):
        self.data = []

    def dataReceived(self, data):
        print "dataReceived %r" % data
        self.data.append(data)

    def readConnectionLost(self):
        print "readConnectionLost"
        self.transport.loseWriteConnection()

    def writeConnectionLost(self):
        print "writeConnectionLost"
        self.transport.loseConnection()

    def connectionLost(self, reason):
        print "connectionLost"


class ListenerFactory(protocol.Factory):
    protocol = ListenerProtocol1


#from twisted.internet import epollreactor
#epollreactor.install()

from twisted.internet import reactor
port = reactor.listenTCP(5555, ListenerFactory())
reactor.run()

Then run

python -c 'import time, socket; s=socket.socket(); 
s.connect(("localhost", 5555)); s.shutdown(1); time.sleep(5)'

Appears to only exhibit in the select reactor on Linux, not in epoll, poll, not select on OSX.

This is because Linux 'select' stops indicating writeability for the fd after you call shutdown(SHUT_WR).

However, twisted already contains the workaround for this, in twisted/internet/abstract.py loseConnection: if self._writeDisconnected is true, it disconnects immediately, instead of waiting for writeability. That is all that's necessary here...

This doesn't properly trigger, however, because in doWrite(), it sets _writeDisconnected = True *after* self._closeWriteConnection() instead of *before*. I think swapping those two statements will fix the bug.

Should be easy to add the test case to the test suite and verify that that change doesn't break anything else, so adding easy keyword. :)

Changed 7 years ago by Zubin Mithra

Attachment: 3037.patch added

All the changes mentioned above have been made.

comment:5 Changed 7 years ago by Zubin Mithra

Keywords: review added
Owner: jknight deleted

The patch to be reviewed is 3037.patch . A new test twisted/internet/test/test_abstract.py has been added.

comment:6 Changed 7 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Igor

The test needs to be re-written to be a unit test - subclass ReactorBuilder or add the methods to an existing class in twisted/internet/test/test_tcp.py. As it is written now, the test will corrupt global state and hang indefinitely (at best).

assigning to igorsobreira because he expressed some interested on IRC in working on this.

comment:7 Changed 6 years ago by itamarst

Author: itamarst
Branch: branches/select-halfclose-3037

(In [33104]) Branching to 'select-halfclose-3037'

comment:8 Changed 6 years ago by Itamar Turner-Trauring

Keywords: review added; easy removed
Owner: Igor deleted

I couldn't reproduce the error on my machine, but it did happen on a bunch of the buildslaves; the proposed fix did indeed work.

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/select-halfclose-3037

comment:9 Changed 6 years ago by jesstess

Owner: set to jesstess

comment:10 Changed 6 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: changed from jesstess to Itamar Turner-Trauring

I talked with Itamar about this after being unable to reproduce on Linux as well, and he said all the Windows buildslaves and some of the Linux ones would fail on the test case.

  • "I was previously asked to to" ==> "I was previously asked to"

Other than that, looks good, please merge.

comment:11 Changed 6 years ago by itamarst

Resolution: fixed
Status: newclosed

(In [33142]) Merge select-halfclose-3037.

Author: mwicked, jknight, itamar Review: jesstess Fixes: #3037

Fix edge-case where notification of half-close never happens.

Note: See TracTickets for help on using tickets.