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

Make test 'iter_panic_fuse' deterministic #674

Merged
merged 4 commits into from
Jul 18, 2019

Conversation

Aaron1011
Copy link
Contributor

Fixes #667

'panic_fuse' will only stop other threads 'as soon as possible' -
if the other threads are fast enough, they might end up processing
the entire rest of the iterator.

This commit changes the test 'iter_panic_fuse' to properly take this
into account, by creating a custom threadpool with only 1 thread.
This makes the test deterministic - with only one thread, the panic
is guaranmteed to be observed when the next item is processed, causing
the desired early exit.

Fixes rayon-rs#667

'panic_fuse' will only stop other threads 'as soon as possible' -
if the other threads are fast enough, they might end up processing
the entire rest of the iterator.

This commit changes the test 'iter_panic_fuse' to properly take this
into account, by creating a custom threadpool with only 1 thread.
This makes the test deterministic - with only one thread, the panic
is guaranmteed to be observed when the next item is processed, causing
the desired early exit.

I've also added the 'more-asserts' crate as a dev dependency, so
that we can print out a more informative error message in
'iter_panic_fuse'
@Aaron1011 Aaron1011 force-pushed the fix/iter_panic_fuse branch from 6d7bfe6 to 9532eb5 Compare July 7, 2019 18:37
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

Thanks. It feels sad to force single-threaded, but I think we're still performing a effective test anyway.

Fixes #667

That issue also mentions check_partial_cmp_short_circuit -- care to address this?

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

sgtm once @cuviper is happy

@cuviper
Copy link
Member

cuviper commented Jul 18, 2019

Thanks!

That issue also mentions check_partial_cmp_short_circuit -- care to address this?

I added a commit for this myself.

bors r+

bors bot added a commit that referenced this pull request Jul 18, 2019
674: Make test 'iter_panic_fuse' deterministic r=cuviper a=Aaron1011

Fixes #667

'panic_fuse' will only stop other threads 'as soon as possible' -
if the other threads are fast enough, they might end up processing
the entire rest of the iterator.

This commit changes the test 'iter_panic_fuse' to properly take this
into account, by creating a custom threadpool with only 1 thread.
This makes the test deterministic - with only one thread, the panic
is guaranmteed to be observed when the next item is processed, causing
the desired early exit.

Co-authored-by: Aaron Hill <[email protected]>
Co-authored-by: Josh Stone <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 18, 2019

@bors bors bot merged commit 2a2c5a8 into rayon-rs:master Jul 18, 2019
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.

Intermittent test failures on master
3 participants