twisted-trac
commented
13 years ago  |
Automation removed owner |
|---|
Closed twisted-trac closed 19 years ago
twisted-trac
commented
13 years ago |
Automation removed owner |
|---|
twisted-trac
commented
19 years ago  |
@exarkun commented |
|---|
#!html
<pre>
Flipped the order of dataReceived and resumeProducing
Resolved by r13826
</pre> |
andrea commented |
|---|
#!html
<pre>
what is being described is receiving data from network, stopping the protocol
while doing an sql query on the received data. Once the sql query completes and
the callback fires, we resume the protocol, and we receive more data, that in
turn make us stop the protocol again and execute another sql query.
The first sql callback firing and completeting will execute this sequence to
re-enable input:
self.transport.resumeProducing # resume the input
calls self.dataReceived # resumeProducing will process the
buffer
calls self.stringReceived # there's something in the
buffer
# string received got the 'string'
# while processing the string we stop the
producer again
calls self.stopProducing
calls self.transport.stopProducing
runs some sql query again
#return from stringReceived
# here we're back after calling dataReceived
calls self.transport.resumeProducing
# this resumeProducing is invalidating the above stopProducing
</pre> |
spiv commented |
|---|
#!html
<pre>
Andrea Arcangeli reported that:
"""
Ok here we go, I even found a severe bug in the linereceiver code and I
fixed it as well. The bug in linereceiver happens like this:
resumeProducing()
calls self.transport.resumeProducing
calls self.dataReceived
calls self.stringReceived
calls self.stopProducing
calls self.transport.stopProducing
runs some sql query again
calls self.transport.resumeProducing <- BUG, invalidated the above stopProducing
The above bug would for sure screwup two sql queries in a raw (i.e.
the deferred callback executing another sql query). Fix is the below one
liner, it's enough to call dataReceived as the last thing in
resumeProducing (I assume for the lower layer the effect of
self.transport.resumeProducing; self.transport.stopProducing will be a
noop as long as we never schedule in between, i.e. never return in
between)
"""
See http://twistedmatrix.com/pipermail/twisted-python/2005-February/009626.html
for more details.
At very least, there should be a test case written to prove or disprove this.
</pre> |
@exarkun commented |
|---|
#!html
<pre>
I don't think we can fix this. I am having great difficulty understanding the
report, but I believe what is being described is the parallel processing of two
requests, and the inability to use transport.pauseProducing() to prevent this
from happening.
Since arbitrary buffering may occur at the protocol level,
transport.pauseProducing() is not a feasible way to support this use-case.
Instead, more arbitrary buffering needs to be performed.
We probably need some documentation explaining this, as well as how to implement
something which is correct in the face of contraints for serialization. There
are a few places in Twisted which do this, but they are pretty well buried, and
I do not think any documentation material explicitly covers this.
</pre> |
@exarkun commented |
|---|
#!html
<pre>
Ok, this makes more sense now. Have you consider this scenario, though?
dataReceived is called with one string that contains two entire lines worth of
input. It splits up the input and calls stringReceived with the first line.
stringReceived pauses the producer and runs some SQL query, then returns to
dataReceived. dataReceived then calls stringReceived immediately again with the
second line of input. stringReceived pauses the producer again (which does
nothing, since it was paused already) and runs another SQL query (an error,
because now two queries are running in parallel).
Does this cause the same kind of problem as the scenario you described? If so,
I think it indicates the need for a fix which is unrelated to
stopReading/startReading.
</pre> |
andrea commented |
|---|
#!html
<pre>
"dataReceived then calls stringReceived immediately again"
the above can't happen, stringReceived can only be called once, this is the
whole point of calling pauseReceived, no?
</pre> |
@exarkun commented |
|---|
#!html
<pre>
Not quite. It stops(*) the transport from grabbing any more bytes from the
kernel, but that does nothing to prevent bytes that have already entered
userspace from being passed to your methods. If a packet arrives with two
entire lines in it, both lines will almost certainly be read into userspace at
the same time, so there is no way to "pause" stringReceived in between them
using pauseProducing. The only way to avoid processing the 2nd until you have
finish processing the 1st is to make your stringReceived implementation aware of
this possibility and behavior appropriately. "Appropriately" can vary from case
to case, but one strategy is to track internally whether an event is already
being processed and queue up subsequent events manually until they can be processed.
A general implementation of this pattern probably belongs in Twisted, but
unfortunately there is not one currently.
(*) It may not actually totally stop things immediately. For example, the IOCP
reactor may have initiated an overlapped operation _before_ you call
pauseProducing which complets _after_ you call pauseProducing which will deliver
bytes to your protocol.
</pre> |
andrea commented |
|---|
#!html
<pre>
this is not how I read the code and I thought I even tested it works as I
expected (not in linereceived, but in in32stringreceiver)
while self.line_mode and not self.paused
the above while loop will break immediatly as soon as paused is set.
why = self.lineReceived(line)
the above will set paused = True
so linereceived/stringreceived will only be called once with a single
line/string until resumeproducing is called.
</pre> |
@exarkun commented |
|---|
#!html
<pre>
Ahhh. LineReceiver.pauseProducing. Sorry, I have been thinking about direct
calls to the transport's pauseProducing. You're right,
LineReceiver.pauseProducing will prevent the delivery of any further lines.
Getting back to the original issue you raised, then: one solution would be to
keep track of the number of times pause and resume have been called. I'm going
to try to come up with a unit test to exercise this functionality and see if the
straightforward change (use an int counter for paused instead of a boolean)
fixes it without breaking anything else.
</pre> |
andrea commented |
|---|
#!html
<pre>
What is it getting broken by calling self.transport.resumeProducing before
self.dataReceived('') as in my patch? That's enough to fix it for me IMHO.
</pre> |
andrea commented |
|---|
#!html
<pre>
On Sun, May 22, 2005 at 08:51:56PM +0000, Jp Calderone [Twisted issue tracker] wrote:
>
> Jp Calderone <exarkun@...> added the comment:
>
> Flipped the order of dataReceived and resumeProducing
>
> Resolved by r13826
thanks JP!
</pre>
Searchable metadata
``` trac-id__899 899 type__defect defect reporter__spiv spiv priority__high high milestone__ branch__ branch_author__ status__closed closed resolution__fixed fixed component__core core keywords__ time__1109068521000000 1109068521000000 changetime__1116822251000000 1116822251000000 version__ owner__ cc__exarkun cc__spiv cc__itamarst cc__jknight cc__hypatia cc__andrea ```