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

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.

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.

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.

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

HTH,

Alex.
Received on Fri Sep 16 2011 - 04:29:52 MDT