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

Change FAQ example with race conditions #1150

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

JulianKnodt
Copy link
Contributor

Previously this example in the FAQ as written would've introduced a race condition.
While I know the objective of this example is not to show exactly correct code, it seemed a bit odd to me since there's a whole section earlier about preventing race conditions. The last change was a while ago, so the API wasn't the same, but it would be good to update it so there's no race conditions.

@JulianKnodt JulianKnodt changed the title Change example with race conditions Change FAQ example with race conditions Mar 31, 2024
FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated Show resolved Hide resolved
FAQ.md Outdated
best_cost.store(...);
// Note: doing `best_cost.store(...)` will introduce race conditions.
// Using `fetch_*` such as `fetch_min` will help prevent race conditions.
best_cost.fetch_min(cost_so_far, Ordering::SeqCst);
Copy link
Member

Choose a reason for hiding this comment

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

The idea is that best_cost is a total, while cost_so_far is only partial. So the first load means "if we're already too big, give up," and then it would finish computing the current cost and eventually try to fetch_min that as the new best.

Suggested change
best_cost.fetch_min(cost_so_far, Ordering::SeqCst);
best_cost.fetch_min(..., Ordering::SeqCst);

Or we could expand the first ellipses, something like:

    let total_cost = cost_so_far + ...;
    best_cost.fetch_min(total_cost, Ordering::SeqCst);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with the first suggestion of changing fetch_min(..., Ordering::SeqCst).

@cuviper
Copy link
Member

cuviper commented Apr 4, 2024

Thanks!

@cuviper cuviper enabled auto-merge April 4, 2024 23:52
@cuviper cuviper added this pull request to the merge queue Apr 4, 2024
Merged via the queue into rayon-rs:main with commit 9f1e96a Apr 4, 2024
4 checks passed
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