Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race in MapParallelOrderedObservable #906

Merged
merged 1 commit into from Jun 19, 2019

Conversation

@rfkm
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

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 approved these changes Jun 19, 2019

@Avasil Avasil merged commit 4e7514a into monix:master Jun 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.