Re: [Bug 2038] check reply_body_max_size before ICAP

bugzilla-daemon_at_squid-cache.org wrote:
> http://www.squid-cache.org/bugs/show_bug.cgi?id=2038
>
>
> Alex Rousskov <rousskov_at_measurement-factory.com> changed:
>
> What |Removed |Added
> ----------------------------------------------------------------------------
> Attachment #1609 is|0 |1
> obsolete| |
> Attachment #1624 is|0 |1
> obsolete| |
> AssignedTo|alexey.veselovsky_at_eykontech.|rousskov_at_measurement-
> |com |factory.com
>
>
>
>
> --- Comment #16 from Alex Rousskov <rousskov_at_measurement-factory.com> 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