Skip to content

New Exchanger Implementation #396

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

Merged
merged 2 commits into from
Aug 18, 2015
Merged

New Exchanger Implementation #396

merged 2 commits into from
Aug 18, 2015

Conversation

jdantonio
Copy link
Member

Brand new implementation based on OpenJDK 8

@jdantonio jdantonio added this to the 2.0.0 Release milestone Aug 13, 2015
@jdantonio jdantonio added the bug A bug in the library or documentation. label Aug 13, 2015
@jdantonio jdantonio self-assigned this Aug 13, 2015
@jdantonio jdantonio force-pushed the exchanger branch 4 times, most recently from a22dd3f to 980e779 Compare August 13, 2015 14:28
@jdantonio jdantonio changed the title [DO NOT MERGE] Exchanger New Exchanger Implementation Aug 13, 2015
@jdantonio jdantonio changed the title New Exchanger Implementation [DO NOT MERGE] New Exchanger Implementation Aug 13, 2015
@jdantonio jdantonio force-pushed the exchanger branch 4 times, most recently from f5bdb0a to 4add0c3 Compare August 13, 2015 18:02
@jdantonio jdantonio changed the title [DO NOT MERGE] New Exchanger Implementation New Exchanger Implementation Aug 13, 2015
@jdantonio
Copy link
Member Author

I believe this new implementation is functionally correct. Both the original condition variable implementation and the later MVar implementation had known bugs, which is why we moved Exchanger into Edge. So I don't think there is any harm in merging it, even if we discover bugs later.

@mighe @chrisseaton @pitr-ch I know you are all very busy, but if any of you have a chance to review this PR I would appreciate it.

@chrisseaton
Copy link
Member

The crucial method for anyone else reviewing this: https://github.com/ruby-concurrency/concurrent-ruby/pull/396/files#diff-8a5aa6ad7f52e71c4ce9a64573329694R163

I don't understand the high-level algorithm here so it's hard to verify that it's a correct algorithm, or that it's correctly implemented. Can you describe the algorithm in a comment, or reference the code where it is from?

@jdantonio
Copy link
Member Author

@chrisseaton Done. The algorithm is described (in detail) in the source. Once I wrote it all out I discovered a bug so that was a bonus.

@mighe
Copy link
Contributor

mighe commented Aug 14, 2015

I've read carefully both the algorithm and the code, I've also run the code "by hand" looking for race conditions.
It seems ok, I could not find any issue, but I'd like to review again everything on Monday to be sure that it is really ok :)

@jdantonio
Copy link
Member Author

This PR has been rebased against the latest master (after thread_safe was merged). I would like to include this PR in the 1.0.0.pre1 release but that is not essential. I'll leave it open for a couple of days to get more review/feedback. We'll release 1.0.0.pre1 next week with or without this PR.

jdantonio added a commit that referenced this pull request Aug 18, 2015
@jdantonio jdantonio merged commit 28f6d35 into master Aug 18, 2015
@jdantonio jdantonio deleted the exchanger branch August 18, 2015 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug in the library or documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants