Re: /bzr/squid3/trunk/ r11740: Do not let cache manager requests kill SMP Squid using isOpen() assertion.

On 16/09/11 16:29, Alex Rousskov wrote:
> On 09/15/2011 08:24 PM, Amos Jeffries wrote:
>> On 16/09/11 06:16, Alex Rousskov wrote:
>>> ------------------------------------------------------------
>>> revno: 11740
>>> committer: Alex Rousskov<rousskov_at_measurement-factory.com>
>>> branch nick: trunk
>>> timestamp: Thu 2011-09-15 12:16:59 -0600
>>> message:
>>> Do not let cache manager requests kill SMP Squid using isOpen()
>>> assertion.
>>>
>>> As the comment above the close call implies, we have not imported
>>> the foreign socket descriptor into our fd_table yet. We must use
>>> raw close(2), just like the corresponding
>>> Mgr::Request::Request(msg) code that allocates request.conn, uses
>>> raw assignment to give that half-baked connection a descriptor.
>>>
>>> TODO: This direct manipulation of Connection::fd is ugly, and this
>>> half-baked connection will most likely cause more [hidden] problems
>>> down the road. For example, Mgr::Request destructor will assert in
>>> a similar way if the request object is destroyed before
>>> Action::respond() is called.>> modified:
>>> src/mgr/Action.cc
>
>
>> Problem is: why is the FD getting outside the IPC Coordinator/Strand
>> scope when it is not "valid" according to fd_table?
>
>
> It is not 100% clear where that IPC scope ends in this case, but the
> problem is not really with the scope borders. The bug was due to the
> [old] action creator saying "we are not going to import this raw fd so
> that each specific action can decide whether it actually needs an fd"
> and the [new] default action code saying "we are going to close this
> Comm connection" even though the Connection object was in that
> half-baked state.
>
>
>> - If it can't be read/write/close, it should not really be passed round
>> at all.
>> - If it *can* be read/write/close, the ->close() is the right way to
>> close and remove the possible handlers.
>
> It can be read/written/closed, but it can be read/written/closed using
> Comm only after the fd is imported into Comm space. The old code did not
> import and used raw close(2) by default. The new code still did not
> import but used Comm close by default.
>
>
>> Correlation of these is that the code doing conn->fd = msg.getFd() when
>> unpacking the TypedMsgHdr should be (a) not, or (b) setting up fd_table
>> state sufficiently to match the Comm::Connection state.
>

Option (1)

> Yes, that is why I left a TODO note: We can change the old code to
> import the fd into Comm [and create a full Connection object] as soon as
> possible. It would be a waste of cycles in most cases, but it will
> prevent half-baked Connection objects from lingering around and causing
> more asserts or fd leaks.
>

option (2)
> The alternative is to use a "raw fd holder" class/object that would use
> raw close(2) unless a fully-baked Connection object is created. SSL code
> uses that approach for many temporary SSL things to avoid leaks.

option (3):
  Comm::Connection::close() could detect the fd_table flag which is
asserting right? then it could use ::close() instead of comm_close().

option (4):
  Or probably, add another Comm::Connection flag to indicate "partial
import" for the opening TCP flags. Which triggers the particular closing
method if still set on ->close().

>
> Since this is not a performance-sensitive area of the code, the former
> (i.e., create a Connection object ASAP) may be better/simpler.

Well, like you keep reminding me. Everything is performance sensitive
indirectly.

So I'm thinking option (4), with option (1) in specific cases would be best.
   Option (3) whenever we fiddle with the fd directly. Option (1)
whenever its known that the FD will be used. Possibly some time after
option (3) was done, so unsetting the half-imported flag if set earlier.
  This way we avoid fd_table and related CPU consumptions entirely for
most of these. And still roughly follow the semantics of what the flags
mean: things done when opening this FD, so we can repeat the sequence in
comm_reset() as needed.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.15
   Beta testers wanted for 3.2.0.11