Avoid using exceptions for control flow in GetBatch #2138
Conversation
| Contracts.AssertValue(inner); | ||
| throw Contracts.ExceptDecode(inner, "Stream reading encountered exception"); | ||
| if (!_queue.TryTake(out LineBatch batch, millisecondsTimeout: -1)) | ||
| return default; |
There was a problem hiding this comment.
I'm not familiar with BlockingCollection, so please excuse my ignorance.
But is IsAddingCompleted the only reason TryTake will return false when given an Infinite timeout?
There was a problem hiding this comment.
Is IsAddingCompleted the only reason TryTake will return false when given an Infinite timeout?
Yeah... well, or a Dispose.
(Related, I think there may be some additional exceptional issues happening because of disposal in fact. I'm seeing a non-trivial number of OperationCanceledExceptions getting thrown and swallowed inside of BlockingCollection due to it being disposed of while there are still threads actively waiting to take. While it'd be great if BC could avoid using exceptions itself for control flow in that case, it's generally not something to optimize for given you're generally not supposed to dispose of things while they're still in use.)
ghost
locked as resolved and limited conversation to collaborators
Fixes #2137
Contributes to #2099
This avoids using Take() in GetBatch, instead using TryTake with an infinite timeout, the only difference being whether it expects to eventually get data, and thus whether it throws or returns false when it finds the collection empty and marked for completion. This doesn't fix #2099, but it helps. These exceptions are largely a side-effect of tons of threads getting created, which is another huge contributor to #2099.
cc: @TomFinley