Turn on _enableOffering in TryReceiveAll as in TryReceive #203
Conversation
In SourceCore TryReceive turns on enableOffering to offer the next messages to any target (if there is one) while TryReceiveAll didn't. Fix #202
| @@ -351,6 +351,9 @@ internal bool TryReceiveAll(out IList<TOutput> items) | |||
| // Increment the next ID. Any new value is good. | |||
| _nextMessageId.Value++; | |||
|
|
|||
| // Now that the next message has changed, reenable offering if it was disabled | |||
| if (!_enableOffering) _enableOffering = true; | |||
There was a problem hiding this comment.
Why not set it unconditionally? Surely that's faster.
There was a problem hiding this comment.
Is there any cause for concern that offering shouldn't be enabled, for example in the case of a faulted block?
There was a problem hiding this comment.
I agree completely about the condition. It was simply copied verbatim from the TryReceive method and the guideline says to keep the original style. They can both be changed however.
There was a problem hiding this comment.
About the faulted block, I don't think so since:
TryReceivealready has the same behaviour of increasing the_nextMessageIdby one, enabling_enableOfferingand callingCompleteBlockIfPossible.- Completion is handled just after by
CompleteBlockIfPossibleand further inner calls that check_decliningPermanentlyandCanceledOrFaulted
|
@i3arnon, thanks for the fix! The fix itself looks good. The test, however, is insufficient, as the bug is due to a race condition (on my machine looping over your test before the fix, it only fails ~1% of the time). I'd suggest changing the test by wrapping up the logic before the WhenAny into a Task-returning method that returns the OutputAvailableAsync task, and then in the test method instead of setting outputAvailableAsync to a single instance, set it to something like "Task.WhenAll(Enumerable.Range(0, 1000).Select(_ => Repro()))", where Repro is the method that returns the OutputAvailableAsync task. That way, it's much more likely that we'll see the failure but without taking significantly more time for the test, as we'll be doing all of the tests effectively concurrently. Does that make sense? As for the "if (!_enableOffering) _enableOffering = true", we can probably change all of the occurrences of this pattern in this block to just be "_enableOffering = true". I don't remember now why we thought this was a good idea (maybe something related to invalidating cache lines, though that doesn't look particularly relevant here), and some quick tests just now show no benefit from the extra check. |
|
Thanks @stephentoub, I've reimplemented the test as you suggested and replaced all the conditional sets of |
| Assert.True(completedTask != timeoutTask); | ||
| } | ||
|
|
||
| private Task GetOutputAvailableAsyncTaskAfterTryReceiveAllOnNonEmptyBufferBlock() |
There was a problem hiding this comment.
This could also be done as a lambda to keep it inside of the corresponding [Fact] method, but I'm ok with it as is.
|
Thanks for making the changes to the test. LGTM. |
Turn on _enableOffering in TryReceiveAll as in TryReceive
In SourceCore TryReceive turns on enableOffering to offer the next messages to any target (if there is one) while TryReceiveAll didn't.
Fix #202