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

[Kim Barrett]

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