On 15/07/2014 5:02 a.m., Alex Rousskov wrote:
> On 07/14/2014 03:48 AM, Amos Jeffries wrote:
>>   Converts the PortCfgPointer to reference counted
> 
> This change should have gone through an audit IMO, but I appreciate
> having an entire Sunday to react to the "will commit shortly" notice for
> a not yet posted patch.
> 

The patch was posted on 23rd June (3 weeks). Only changes since then
were library link dependencies on unit tests. Sorry that the reminder
turned out to be Sunday. I keep forgetting the weekend overlap lag.

> 
>> -    if (!s.valid()) {
>> +    if (!s) {
>>          // it is possible the call or accept() was still queued when the 
>> port was reconfigured
>>          debugs(33, 2, "HTTP accept failure: port reconfigured.");
>>          return;
> 
> While the port pointer could get invalidated in the [fixed] old code, it
> cannot become nil in the new code. The above comment is no longer valid
> and the code itself no longer works as intended. Please fix both
> instances of the above logic: one in httpAccept() and one in httpsAccept().
> 
> AFAICT, if you simply replace the above code with a comment warning that
> the port may no longer be in the current configuration, we will continue
> to start transactions previously accepted on no longer configured ports.
> Hopefully, this will not cause any new problems, but there may be a
> better solution.

Good catch. I think this is actually dead code now and can be removed
completely now that it is a ref-counted port. Do you agree on that?

Amos

Reply via email to