Skip to content

Fix race in MapParallelOrderedObservable#906

Merged
Avasil merged 1 commit into
monix:masterfrom
rfkm:fix-race-map-parallel-ordered
Jun 19, 2019
Merged

Fix race in MapParallelOrderedObservable#906
Avasil merged 1 commit into
monix:masterfrom
rfkm:fix-race-map-parallel-ordered

Conversation

@rfkm
Copy link
Copy Markdown
Contributor

@rfkm rfkm commented Jun 17, 2019

MapParallelOrderedObservable could be stuck forever.

Consider the following scenario:

  1. get out of the while loop in sendDownstreamOrdered as queue is empty, but sendDownstreamLock is being still locked
  2. another item A comes from upstream and is enqueued to queue
  3. A's future is completed, and then sendDownstreamOrdered is invoked
  4. sendDownstreamLock is still locked, so the second call of sendDownstreamOrdered caused by A is rejected
  5. sendDownstreamLock is finally unlocked
  6. However, if there is no more item or there is no room in semaphore, there is no chance to kick sendDownstreamOrdered again

This can likely happen especially for small tasks (such tasks are not suitable for this operator in the first place, though). For example, I can reproduce it with the following simple observable:

Observable
  .range(1, 10000000)
  .mapParallelOrdered(10)(Task(_))
  .dump("O")

This patch will ensure that the polling of the queue is done as many as the number of elements.
As far as I have examined, there is no notable performance regression.

I'm not sure but #846 may have made the problem more likely.

@Avasil
Copy link
Copy Markdown
Collaborator

Avasil commented Jun 19, 2019

Thank you!

I wish we could add regression test but it would probably take about a minute so probably not worth it.

@Avasil Avasil merged commit 4e7514a into monix:master Jun 19, 2019
mdedetrich pushed a commit to mdedetrich/monix that referenced this pull request Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants