Re: r13497: Converts the PortCfgPointer to reference counted

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
Received on Tue Jul 15 2014 - 05:59:27 MDT