[Twisted-Python] Re-entrancy policy for the reactor

[exarkun at twistedmatrix.com]

On 09:10 pm, glyph at twistedmatrix.com wrote:
>
>On Jul 24, 2011, at 10:51 PM, Itamar Turner-Trauring wrote:
>>I'm going to merge #5063 next time I have a few minutes when I'm more
>>awake, which will mean my yak stack[1] is empty and I can go back to
>>working on abortConnection(). As explained in
>>http://twistedmatrix.com/trac/ticket/78,  abortConnection() is like
>>loseConnection(), except it doesn't wait until buffers are written 
>>out,
>>it closes the connection immediately. The question is, how 
>>immediately?
>>
>>My current implementation, a bit like one of the half-close code 
>>paths,
>>ends up calling connectionLost directly. That means you can have a 
>>call
>>stack that looks like this:
>>
>>MyProtocol.dataReceived
>>TCPConnection.abortConnection
>>...
>>MyProtocol.connectionLost
>>
>>This can lead to re-entrancy bugs. I am tempted to suggest a policy of
>>No Reactor Reentrancy, but that is currently violated by one of the 
>>TCP
>>half-close code paths, and producers if I'm not mistaken.
>>
>>Some options:
>>
>>(a) Leave abortConnection() reentrant.
>>(b) Make abortConnection() non-reentrant, don't change any current 
>>APIs.
>>(c) Make all reactor APIs non-reentrant.
>>
>>(Since there's a ticket for documenting the reentrancy policy, you 
>>will
>>notice I've added another yak to my stack. I'll finish #78 someday!)
>>
>>Thoughts?
>
>My main thought here is that protocol reentrancy is bad, and nobody 
>really expects it even if they think it should be fine.
>
>However, I do believe it would be best (easier to test, in particular) 
>to immediately call connectionLost within doRead (or doWrite) after 
>dataReceived exits, rather than callLater(0)-ing it or otherwise 
>placing it into a global call queue.

Probably not easier to test, nor easier to get right.  A single 
generalized solution to avoid re-entrancy is probably the only way to 
avoid a perpetual sequence of accidental re-entrant calls in obscure 
untested cases in each different reactor implementation.

Perhaps the solution doesn't need to be `reactor.callLater(0, ...)`, but 
I think various implementations will be better if they put tasks into 
some kind of queue, perhaps checked at the end of the iteration instead 
of in the next iteration, rather than special casing each possible re- 
entrant event in each possible event dispatcher.

Jean-Paul