Opened 9 years ago

Last modified 3 years ago

#6461 defect new

loseConnection does not wait for producer to finish, contra the documentation

Reported by: Itamar Turner-Trauring Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: Itamar Turner-Trauring Branch:
Author:

Description

The the servers howto says "If a producer is being used with the transport, loseConnection will only close the connection once the producer is unregistered.". This is not only inaccurate, but what's worse the actual behaviour is non-deterministic and different depending on whether it's a streaming or non-streaming producer. See FileDescriptor.doWrite for the horrid details.

Attachments (1)

producer.py (564 bytes) - added by Itamar Turner-Trauring 9 years ago.
One broken behavior out of many

Download all attachments as: .zip

Change History (11)

comment:1 Changed 9 years ago by Glyph

:-O

comment:2 Changed 9 years ago by Glyph

Can you share some of the horrid details here? They're not immediately obvious based on a reading of the code, unfortunately. (I mean, the horridness is. The details aren't.)

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

Can you share some of the horrid details here?

Yes, please do. If you arrived at this conclusion by logic, then it'd be really nice if you'd outline the steps of that logic. If you arrived at the conclusion by observing it in reality, then mentioning that would be helpful.

Perhaps you know what's going on here and will take care of the issue, but at best that still means this ticket serves as a bad example for other contributors.

comment:4 Changed 9 years ago by Glyph

Cc: Itamar Turner-Trauring added

Adding itamar in case he's not already on here. (Hint, hint.)

comment:5 Changed 9 years ago by Jean-Paul Calderone

Suggest closing as "works for me" at this point. Please provide more information if you'd like to avoid this.

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

Sigh. Trac didn't email me anything until I was CC'ed, so I didn't see original question.

Since it's not obvious from reading FileDescriptor.doWrite, attached see an example. Notice we never unregister the producer.

Changed 9 years ago by Itamar Turner-Trauring

Attachment: producer.py added

One broken behavior out of many

comment:7 Changed 6 years ago by Glyph

Status: newassigned

comment:8 Changed 5 years ago by Glyph

Owner: changed from Glyph to Cory Benfield
Status: assignednew

Hey Cory. Did you manage to fix this accidentally on any of your many travels through this code?

comment:9 Changed 5 years ago by Cory Benfield

Owner: changed from Cory Benfield to Glyph

I don't believe I fixed it anywhere as the result of my own actions. However, I *do* believe that TLSMemoryBIOProtocol *does* implement this behaviour. However, my understanding is that that's not what Itamar was concerned about (not least because TLSMemoryBIOProtocol didn't exist 4 years ago), but Itamar was instead concerned about the behaviour of anything inheriting from abstract.FileDescriptor.

However, Itamar is right that a fairly trivial code-read reveals the problem: see abstract.FileDescriptor.loseConnection, which pays no heed to whether a producer is registered. To satisfy those who want sample code to reproduce a problem, here is a simple Twisted server that demonstrates the problem by registering itself with a transport and then calling transport.loseConnection:

from zope.interface import implementer

from twisted.internet.interfaces import IPushProducer
from twisted.internet.protocol import Protocol, Factory
from twisted.internet.endpoints import TCP4ServerEndpoint
from twisted.internet import reactor


@implementer(IPushProducer)
class TestProtocol(Protocol):
    def connectionMade(self):
        # This should prevent the connection going away.
        print("Connection made, registering self with transport.")
        self.transport.registerProducer(self, True)
        reactor.callLater(1, self.doTheThing)

    def pauseProducing(self):
        print("Pausing production")

    def stopProducing(self):
        print("Stopping production")

    def resumeProducing(self):
        print("Resuming production")

    def connectionLost(self, reason):
        print("Connection lost: %s" % reason)

    def doTheThing(self):
        print("Asking transport to lose connection, not unregistering.")
        self.transport.loseConnection()

ep = TCP4ServerEndpoint(reactor, 8888)
ep.listen(Factory.forProtocol(TestProtocol))
reactor.run()

This client code will demonstrate the problem:

import socket
import time

s = socket.create_connection(('localhost', 8888))
s.send(b'hello')
time.sleep(500)

The twisted server should just hang there forever, but it does not. Instead, it prints this:

Connection made, registering self with transport.
Asking transport to lose connection, not unregistering.
Stopping production
Connection lost: [Failure instance: Traceback (failure with no frames): <class 'twisted.internet.error.ConnectionDone'>: Connection was closed cleanly.
]

This is clearly wrong: the protocol is registered as a producer with the transport, and should therefore prevent loseConnection from doing anything. It does not: instead, the transport closes the connection and calls stopProducing.

Note, as I said above, that this is another difference between TLSMemoryBIOProtocol and abstract.FileDescriptor, as TLSMemoryBIOProtocol does respect this requirement. I'm not sure how you change this: in my experience, I suspect a lot of Twisted users are relying on the actual behaviour rather than the documented one.

comment:10 Changed 3 years ago by Glyph

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