Review Merge, save some allocations. #639
Conversation
… Sink-features and the SafeObserver-class.
| _parent._group.Remove(_self); | ||
| if (_parent._isStopped && _parent._group.Count == 1) | ||
| _parent._group.Remove(this); | ||
| if (_parent._isStopped && _parent._group.Count == 0) |
There was a problem hiding this comment.
Same possible problem as with SelectMany: _isStopped is plain and can be reordered after _group.Count.
There was a problem hiding this comment.
Are there any current open issues that could be explained by this going wrong sometimes? Same for SelectMany.
There was a problem hiding this comment.
Do you want me to post an issue which demonstrates this race condition?
There was a problem hiding this comment.
Now that we're fixing it, no I wouldn't suggest that. Let's ask @onovotny about whether we should put out another preview to have something to point people to, should someone encounter the issue.
There was a problem hiding this comment.
I could not trigger this specific reordering on my machine but I found another issue via #660.
There was a problem hiding this comment.
That's already on the fix branch ?
There was a problem hiding this comment.
The main branch. This PR did not change the algorithm so the test should fail as well. You could try it in this PR as the test is standalone.
There was a problem hiding this comment.
I guess I'll merge this, then make the failing test (and subsequent fix) a (failing) new WIP-PR.
There was a problem hiding this comment.
Tried with your branch. #660 still fails.
No description provided.