[squid-dev] [PATCH] convert DNS shutdown to runner

[Alex Rousskov]

On 10/02/2016 06:35 AM, Amos Jeffries wrote:
> This patch adds a Runner to manage the DNS component state on shutdown
> (and the matching reconfigure port closures). The actions taken on
> reconfigure and shutdown are not changed, just their timing.

> +    RunRegisteredHere(RegisteredRunner::startReconfigure);
> +
>      // Initiate asynchronous closing sequence
>      serverConnectionsClose();
>      icpClosePorts();
>  #if USE_HTCP
>      htcpClosePorts();
>  #endif
> -    Dns::Shutdown();


Have you checked that serverConnectionsClose(), icpClosePorts(), and
htcpClosePorts() (and the processing they trigger) can live without DNS?
I am not saying they cannot. I am just asking whether we are just hoping
that they can or reasonably certain that they can. Either way, please
reflect the answer in the commit message.


> Visible differences are now that Squid logs when DNS ports are being
> closed (and the reason)

> +    debugs(78, DBG_IMPORTANT, reason << ": Closing DNS sockets");

IMHO, there is nothing important about it. Reconfiguration and shutting
down is already logged. The [changing] details of that long process are
not important [to admins]. I suggest changing the debug level to 2, but
I cannot not insist on it -- cache.log is already filled with noise.


> +    debugs(78, DBG_IMPORTANT, reason << ": Closing DNS sockets");

Same here.


> +    /// Meant for halting any active modules that could be triggered by external
> +    /// events (such as listening ports) while changing their configuration.

I suggest something a lot more general, like "Meant for modules that
need to prepare for their configuration being changed [outside their
control]. The changes end with the syncConfig() event."

All of the above can be adjusted during commit IMO.


Thank you,

Alex.