RFR (S): 8230611: infinite loop in LogOutputList::wait_until_no_readers()

[David Buck]

Hi Kim!

Thanks again for helping me to get this fix in shape.

Once again, I agree with everything you said. I have returned the copy 
ctor to my first version (using an initializer list), and have modified 
the copy assignment so that it now exactly matches your number 2 
suggestion from your previous response.

Here is a new (and hopefully final) webrev:

http://cr.openjdk.java.net/~dbuck/8230611_ver02/

I am still running the sanity check builds / tests in the background. 
But seeing how the copy ctor is simply a reversion back to the already 
thoroughly-tested version from earlier, and that the copy assignment 
override is dead code in our builds, I do not expect any surprises.

Does the latest webrev look good? Any other thoughts or feedback?

Cheers,
-Buck

On 2019/11/22 4:08, Kim Barrett wrote:
>> On Nov 21, 2019, at 6:34 AM, David Buck <david.buck at oracle.com> wrote:
>>> I think the move of the call to increase_readers() from
>>> LogOutputList::iterator to the Iterator constructor is potentially a
>>> mistake. I think the counter protection should be increased before the
>>> node is obtained from the _level_start array. It might be that there
>>> are reasons why the proposed change will work, but I think the
>>> original code is safer and easier to analyze.
>>
>> You were right to worry. Looking at the code now, it seems clear to me that the intent was to make sure that the reader count was incremented *before* any fields were initialized. My change breaks that and made a nasty race possible.
> 
> I don’t think it’s quite that simple.  The invariant that must be maintained is that the
> count can’t drop to zero while there are still references (iterators) that might be used.
> 
>> I have returned the call to increase_readers() back to where it originally was.
> 
> Good.
> 
>> I have also modified the new copy ctor to make sure that the count is incremented before the 2 fields.
> 
> No, don’t do that.  Your ver00 copy ctor was fine.  The counted reference from the itr
> argument ensures the count can’t drop to zero before the increment by the copy ctor.
> 
>> Finally, as a small modification of your suggested copy assignment operator, I do not decrement the reader count for the lhs _list until its value has been overwritten. (I do understand that that last one may be a bit overkill…)
> 
> It’s nodes that are being protected, not lists.
> 

-- 
External Email Recipient Confirmation Process - Oracle Internal Only