Changes between and of Initial VersionVersion 5Ticket #1128


Ignore:
Timestamp:
12/20/08 09:50:48 (13 years ago)
Author:
Glyph
Comment:

I find myself strangely ... uncompelled. First off, this code is broken. Unless loseConnection always fires synchronously (and if it does, why do we need this) then self.monarch won't always be None when it hits that assert in appointOneTrueKing. I can't actually tell what you want here - if connection 1 arrives, then connection 2 arrives, and 1 disconnects, but connection 3 arrives before connection 1 finishes going away: who wins, 2 or 3? (Assuming you agree with the proposed placement of this deferred, you also don't upcall to connectionLost; how do you expect Protocol to get notified of the connection being dropped to fire its Deferred.)

Second, this implicitly depends on a potentially confusing required ordering of connectionLost and the Deferred firing.

Third, why not just write it like this?

class MonarchyProtocol(Protocol):
    def connectionMade(self):
        self._cldeferred = Deferred()
        self.factory.appoint(self)
    def abdicate(self):
        result = Deferred()
        self._cldeferred.chainDeferred(result)
        return result
    def connectionLost(self, reason):
        if self.factory.monarch is self:
            self.factory.monarch = None
        self._cldeferred.callback(None)
class MonarchyFactory(Factory):
    protocol = MonarchyProtocol
    def __init__(self):
        self.monarch = None
    def appoint(self, newKing):
        if self.monarch is not None:
            self.monarch.abdicate().addCallback(self.appoint, newKing)
            return
        self.monarch = newKing

This is exactly as long as your example, but I think it's a bit easier to read (in addition to fixing the aforementioned bugs).

Unless somebody wants to provide a very clear justification for this, I am -1 on adding this. I think it's going to be complex and tricky to implement, and I think that it's going to be an attractor for bugs in application code which assume an ordering of events related to this deferred's callback that may not be guaranteed. (I'm also updating the description for posterity.)

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #1128

    • Property Priority changed from normal to low
    • Property Status changed from new to closed
    • Property Component changed from to core
    • Property Resolution changed from to wontfix
  • Ticket #1128 – Description

    initial v5  
     1Protocol should have a wrapper around transport.loseConnection that returns a deferred which is fired when connectionLost() is called.