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

[Amos Jeffries]

On 3/10/2016 1:29 p.m., Alex Rousskov wrote:
> 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.
> 

Those do not require working DNS to receive transactions. But may
require it for later async processing steps, logging at the very least.

While looking into that I found that the above is already a problem
simply due to the DNS sockets being closed by the existing code.
Anything which is partially processed and continues some Calls or events
during the reconfigure delay races against the DNS re-opening and
idnsSednQuery() will drop the query objects if sockets are closed when
they try to lookup, or no NS are configured.

Added an XXX note to highlight that race problem. When DNS is refactored
for actual HotConf that will need to be attended to. For this patch it
is existing behaviour.

> 
>> 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.
> 

Done.

> 
>> +    /// 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."

Done.

> 
> All of the above can be adjusted during commit IMO.
> 

Thanks for the review. Applied as Squid-4 rev.14863.

Amos