Re: [MERGE] Cleanup, the Comm part

Alex Rousskov wrote:
> Cleaned up Comm: comm_close, comm_read_cancel, half-closed monitors,
> leaks.
>
> 1) Comm_close now implements the following API:
>
> Comm_close does not close the descriptor but initiates the following
> closing sequence:
>
> 1) The descriptor is placed in a "closing" state.
> 2) The registered read, write, and accept callbacks (if any) are
> scheduled (in an unspecified order).
> 3) The close callbacks are scheduled (in an unspecified order).
> 4) A call to the internal descriptor closing handler is
> scheduled.
>
> Details of the above steps are being documented separately and will
> become a part of Comm API documentation.
>
> Since all notifications are asynchronous, it is possible for a read or
> write notification that was scheduled before comm_close was called to
> arrive at its destination after comm_close was called. Such
> notification will arrive with COMM_ERR_CLOSING flag even though that
> flag was not set at the time of the I/O (and the I/O may have been
> successful). CommIoCbParams::syncWithComm is used for this. The
> credit for this trick goes to Christos Tsantilas.
>
> Removed fde.flags.closing_ flag as unused.
>
>
> 2) Removed most of the half-closed monitoring code. Old code scheduled
> monitoring reads every main loop iteration, I think. It is possible
> that the assumption was that the handler will be activated and
> cleared once per iteration so that the new read can be scheduled. The
> design could result in conflicts between two monitoring reads and
> possibly between a monitoring read and an active read. There were
> also problems with handling closing descriptors.
>
> I have removed the loop, AbortChecker, and the associated splay
> tree). When user code marks the descriptor as half-closed, Comm now
> simply schedules a monitoring read callback. If the user needs to
> check whether the descriptor was marked, Comm checks whether the
> callback is present. If a user schedules a read when there is
> already a monitoring callback, the monitoring callback is removed.
>
> Renamed user-facing monitoring functions but left compatibility
> wrappers in place to minimize user code changes, for now.
>
> It is possible that the whole half-closed monitoring code will be
> eventually deleted. The above changes are meant to preserve the
> intended functionality (but without coredumps) while the decision is
> being made.
>
>
> 3) Removed _SQUID_LINUX_-only code that would avoid addrinfo destruction
> on connect "errors". Squid seems to be working fine without this
> code. With this code, we leak memory on many connect requests because
> of EINPROGRESS. More work is probably needed to reproduce and fix the
> true cause of the memory corruption observed earlier. Removing the
> workaround will allow us to get more bug reports if the problem is
> still there.
>
>
bb:approve

follows the comment, and looks like a much more efficient way of doing
monitoring.

Amos

-- 
Please use Squid 2.7.STABLE4 or 3.0.STABLE8