Re: r13497: Converts the PortCfgPointer to reference counted

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

FWIW, I lack the magic powers to predict what changes you make, even if
they are tiny. The patch posted in June was marked as rough and
untested. The overall patch direction was OK, so I was waiting for the
final/polished/tested version to start a detailed review.

> Sorry that the reminder
> turned out to be Sunday. I keep forgetting the weekend overlap lag.

It is best to cover weekdays when possible, but posting notices on
Sunday should not be prohibited.

On the other hand, final short-term notices for patches that were not
previously posted (and were not reviewed) are not OK. It is fine to post
a patch, wait a month, and then commit it. It is wrong to post a patch,
promise more changes, wait a month (along with others waiting for those
changes!), and then commit a new patch on the grounds that the changes
were insignificant.

It would be nice to have a shared system of tracking (and, hence,
agreeing on the state of) pending patches/changes. Such conflicts are
more likely when each of us maintains his own queue. I do not know of
any good tools for that, unfortunately, but we can try to use a simple
wiki page.

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

Thank you,

Alex.
Received on Tue Jul 15 2014 - 21:32:20 MDT