[EMAIL PROTECTED] wrote:
http://www.squid-cache.org/bugs/show_bug.cgi?id=2038
Alex Rousskov <[EMAIL PROTECTED]> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #1609 is|0 |1
obsolete| |
Attachment #1624 is|0 |1
obsolete| |
AssignedTo|[EMAIL PROTECTED]|[EMAIL PROTECTED]
|com |factory.com
--- Comment #16 from Alex Rousskov <[EMAIL PROTECTED]> 2008-10-10 10:33:22 ---
Created an attachment (id=1819)
--> (http://www.squid-cache.org/bugs/attachment.cgi?id=1819)
Polished fix for the case when virgin response has no Content-Length but grows
to exceed the limit. The adapted response does not exceed the limit.
Attachment #1624 patch was using ServerStateData after calling
sendBodyIsTooLargeError, which is a bad idea because sendBodyIsTooLargeError
calls abortTransaction, which may destroy the server object if we are not
called via our async call handler (see deleteThis logic).
This polished patch tries to address the situation by throwing an exception
instead of calling abortTransaction, but it runs into a similar problem: we
should not be throwing exceptions if we are not called via our async call
handler. The patch adds a boolean parameter to decide when it is safe to throw.
Using call graph analysis, the author found one potentially unsafe place --
ftpSendPassive -- it was too complicated case to analyze using the graph.
The cleanup code also became more complex because sendBodyIsTooLargeError is no
longer just calling abortTransaction and letting it to cleanup/close.
I think that a different side-step should be taken here before allowing all
this complexity to creep in: The Server code should be reviewed to detect any
cases where a Server object is not access via an async call. If there are just
a few simple cases, they should be eliminated. If there are no direct accesses
anymore, abortTransaction can just throw an exception and
sendBodyIsTooLargeError can continue to call abortTransaction.
One way to check for direct accesses is to make all public methods (except for
basic accessors, if any) protected or private. This will also motivate us to
remove the remaining static call wrappers (but they can be declared as friends
for a quick check).
Alex, on the ftpSendPassive issue:
ftpSendPassive collapses down to a series of parallel paths performing
the same operation with different text code sent over the network.
Only one path is taken through it in any call. It does not nest or
recurse, but completes a single network write pass and exits. Async time
later a new different network read job by the ftpReadPassive() handler
will succeed or call it synchronously with brand new state to try
another path.
ftpSendPassive() is called synchronously by other FTP functions. BUT as
a write action, all callers must return immediately after passing code
control to any ftpSend*() function. Giving the guarantee that finishing
ftpSend*() is identical to finishing the whole callback.
Failures and successes both.
Does that clear up the complexity you found troubling? or was it the
tricker issue of the callers that use it having mixed acync/old styles?
Amos
--
Please be using
Current Stable Squid 2.7.STABLE5 or 3.0.STABLE10
Current Beta Squid 3.1.0.1
