Opened 10 years ago

Last modified 6 years ago

#2215 defect new

If lineReceived returns true value, the connection is shut down and this value used as an error message

Reported by: Marcin Kasperski Owned by:
Priority: lowest Milestone:
Component: core Keywords:
Cc: ivank Branch:

Description (last modified by Glyph)

In short: if lineReceived returns some true value (personally I met this problem when I returned deferred), then the whole connection is aborted, moreover, this value is returned as error message. It seems this behaviour is not documented anywhere.

Code like:

class SomeProtocol(basic.LineReceiver):
   def lineReceived(self, line):
        return defer.succeed(997)    # magic value I can recognize easily

class SomeFactory(protocol.ReconnectingClientFactory):
   def clientConnectionLost(self, connector, reason):
        logger.warn("Connection lost. Reason: %s\nTraceback: %s" % (
            reason.getErrorMessage(), reason.getTraceback()))
        protocol.ReconnectingClientFactory.clientConnectionLost(self, connector, reason)

reactor.connectTCP(HOST, PORT, SomeFactory(...))

Results in:

myapp     : WARNING  Connection lost. Reason: <Deferred at 0xB551E1ECL  current result: 997>
Traceback: Traceback (most recent call last):
Failure: <type 'instance'>: <Deferred at 0xB551E1ECL  current result: 997>

(minimal solution) Document this behavior, so people using lineReceiver know that returning value from this method is forbidden.

(better solution) Remove this ugly convention, use exceptions for error reporting instead, and let people return any results they like.

The crucial place in twisted code seems to be twisted/protocols/, function LineReceiver.dataReceived, which contains sth like that:

     while self.line_mode and not self.paused
           why = self.lineReceived(line)
           if why or self.transport and self.transport.disconnecting:
                    return why

As this is WITHIN the loop, we see that in fact returning anything true from lineReceived aborts the loop.

Two extra notes:

  • it seems there is the same problem with dataReceived, maybe also other functions
  • returning value from lineReceived can be useful for instance for unit testing (personally I got this error when I wanted to unit-test some my lineReceived implementation - and had to return deferred to give test procedure opportunity to sync on it)

Change History (5)

comment:1 Changed 10 years ago by Marcin Kasperski

Type: enhancementdefect

comment:2 Changed 10 years ago by Glyph

Description: modified (diff)
Priority: normallowest

The original reason for this behavior is a micro-optimization to avoid the necessity of raising exceptions in order to drop the connection. It's very old, and probably not terribly effective as an optimization. However, I certainly don't care enough about this to change it, especially given that it might break existing code that relied upon this bizarre convention. For what it's worth, it's a mirror of the same convention in dataReceived.

If you wrote a patch and some tests to change the behavior, maybe I might review it. Documentation would definitely be appreciated either way though.

comment:3 Changed 8 years ago by ivank

Cc: ivank added

comment:4 Changed 6 years ago by <automation>

Owner: Glyph deleted

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

This is quite related to #2491.

Note: See TracTickets for help on using tickets.