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

[David Buck]

Hi Kim!

Thank you for taking a look at this!

I agree with you on all counts. I have addressed all of your concerns 
and have retested. Here is the new webrev:

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

 > If assignments where the _list is the same for both is a common case,
 > it might be worthwhile to avoid the counter manipulation altogether in
 > that case, e.g. (2)

The copy assignment operator never seems to be used in our current 
logging code. But I suppose (2) seems like a better choice as I assume 
it is at least not *unlikely* that future code would do assignment 
between iterators of the same _list.

 > 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 have returned the call to increase_readers() back 
to where it originally was. I have also modified the new copy ctor to 
make sure that the count is incremented before the 2 fields. 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...)

Please let me know what you think and thanks again for all the great 
feedback.

Cheers,
-Buck


On 2019/11/21 16:13, Kim Barrett wrote:
>> On Nov 20, 2019, at 11:51 AM, David Buck <david.buck at oracle.com> wrote:
>>
>> Hi!
>>
>> May I please get a review of this small fix:
>>
>> bug report: https://bugs.openjdk.java.net/browse/JDK-8230611
>> proposed fix: http://cr.openjdk.java.net/~dbuck/8230611_ver00/
>>
>> Cheers,
>> -Buck
> 
> Note that RVO becomes mandatory in C++17. Not that this helps us
> today. Though I'm somewhat surprised that any even vaguely recent
> compiler would fail to do it.
> 
> (Side comment on this code: yet another SMR mechanism.)
> 
> The proposed operator= has some problems:
> 
> (1) Self-assignment bug. Self-assignment is rarely an issue in
> practice, but if it costs nothing to handle, then one should do so.
> The problem is that decreasing the counter before increasing it could
> allow an existing waiter to proceed. Just do the increase first.
> 
> (2) operator= should return *this unless there's a good reason not to.
> 
> (3) There's no reason for the rhs argument to be non-const.
> 
> (4) The indentation is not HotSpot's normal 2-space.
> 
> So a better definition would be (1)
> 
> Iterator& operator=(const Iterator& rhs) {
>    _current = rhs._current;
>    rhs._list->increase_readers();
>    _list->decrease_readers();
>    _list = rhs._list;
>    return *this;
> }
> 
> If assignments where the _list is the same for both is a common case,
> it might be worthwhile to avoid the counter manipulation altogether in
> that case, e.g. (2)
> 
> Iterator& operator=(const Iterator& rhs) {
>    _current = rhs._current;
>    if (_list != rhs._list) {
>      rhs._list->increase_readers();
>      _list->decrease_readers();
>      _list = rhs._list;
>    }
>    return *this;
> }
> 
> I think the copy-swap idiom probably isn't the way to go here.
> Compared to (2) it has fewer counter mods for the rvalue with
> different lists, but more for the lvalue with the same list. It is
> always at least as good as (1) though. But to really minimize counter
> manipulations one wants move-construct and move-assign to handle the
> rvalue case, and we don't have those yet.
> 
> https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom/3279550#3279550
> 
> 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.
> 

-- 
External Email Recipient Confirmation Process - Oracle Internal Only