RFR (S): 8230611: infinite loop in LogOutputList::wait_until_no_readers()
David Buck
david.buck at oracle.com
Thu Nov 21 20:24:16 UTC 2019
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
More information about the hotspot-runtime-dev
mailing list