Re: r13497: Converts the PortCfgPointer to reference counted

On 16/07/2014 9:31 a.m., Alex Rousskov wrote:
> On 07/14/2014 11:59 PM, Amos Jeffries wrote:
>> On 15/07/2014 5:02 a.m., Alex Rousskov wrote:
>>> On 07/14/2014 03:48 AM, Amos Jeffries wrote:
>>>> - 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?
>
> Instead of simply removing the now-dead code, I recommend replacing it
> with a comment which warns us that the port may no longer be in the
> current Config. Preserving that knowledge may be important for triaging
> obscure cases. I do not know of any specific case where this caveat is
> important, but that does not mean there are no such cases.
>
> Dead code is not a big deal, of course. Breaking trunk build when SSL is
> enabled is a bigger deal.
>

Okay. Done that replacement for both ports.

Amos
Received on Wed Jul 16 2014 - 12:12:28 MDT