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

Add specs for Promise#zip/Promise.zip ordering #660

Merged
merged 3 commits into from Feb 24, 2018

Conversation

ianks
Copy link
Contributor

@ianks ianks commented Jun 6, 2017

I noticed there were not any specs on the ordering of Promise.zip. Since it is part of the spec (as it applies to Promise.all), I figured I could add some tests for that.

For more info: https://stackoverflow.com/a/28066851

Copy link
Contributor

@hobodave hobodave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't rely on sleep for thread synchronization and ordering. Use concurrency primitives instead.

it 'preserves ordering of the executed promises' do
promise1 = Promise.execute do
# resolves after the second promise
sleep 0.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't rely on sleep for thread synchronization/ordering. Instead use concurrency primites:

running = Mutex.new
cond = ConditionVariable.new
cond2 = ConditionVariable.new

p1 = Concurrent::Promise.execute do
  running.synchronize {
    cond.wait(running)
    'one'
  }
end

p2 = Concurrent::Promise.execute do
  running.synchronize {
    cond2.wait(running)
    'two'
  }
end

p3 = Concurrent::Promise.execute do
  running.synchronize {
    'three'
  }
end

cond2.signal
cond.signal

result = Concurrent::Promise.zip(p1, p2, p3).value
expect(result) .to eq(['one', 'two', 'three'])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hobodave is right. @ianks could you update the spec? I would like to include it since this is indeed the expected behavior.

@pitr-ch pitr-ch added this to the 1.1.1 milestone Jul 23, 2017
@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label Jul 23, 2017
@ianks
Copy link
Contributor Author

ianks commented Nov 16, 2017

@hobodave @pitr-ch strangely, i am getting deadlocks using the cv implementation in the spec.

@pitr-ch
Copy link
Member

pitr-ch commented Nov 18, 2017

@ianks could you use Countdouwnlatch.new(1) instead? It's what we usually use and it could resolve the deadlock without digging in.

@pitr-ch pitr-ch modified the milestones: 1.1.1, 1.1.0 Feb 21, 2018
@ghost ghost assigned pitr-ch Feb 24, 2018
@ghost ghost added the in progress label Feb 24, 2018
@pitr-ch
Copy link
Member

pitr-ch commented Feb 24, 2018

I've updated the PR to use count-down-latch and rebased.

@pitr-ch pitr-ch merged commit 952f56b into ruby-concurrency:master Feb 24, 2018
@ghost ghost removed the in progress label Feb 24, 2018
@ianks ianks deleted the promise-zip-ordering branch February 27, 2018 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants