Skip to content

Synchronization 3.update #296

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
May 25, 2015
Merged

Synchronization 3.update #296

merged 17 commits into from
May 25, 2015

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented May 13, 2015

not ready for merge

@pitr-ch pitr-ch added the enhancement Adding features, adding tests, improving documentation. label May 13, 2015
@pitr-ch pitr-ch added this to the 0.9.0 Release milestone May 13, 2015
jdantonio
jdantonio reviewed May 13, 2015
View reviewed changes
doc/synchronization.md
- Method definition
- Library requirement

It's best practice thought to eager load before going into parallel part of an application.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: though

@jdantonio jdantonio mentioned this pull request May 14, 2015
Merged
@jdantonio
Copy link
Member

Have you had a chance to see the request from the Rails team regarding enhancements to condition variables? Is that something you think we should add? If so I'd like to do it in v0.9 so that the Rails team can move forward. We can push back the release as long as necessary. Thoughts?

@pitr-ch pitr-ch force-pushed the synchronization branch from 363c2c0 to e9c9a76 Compare May 16, 2015 16:58
@pitr-ch pitr-ch force-pushed the synchronization branch from e9c9a76 to 4b5cb7c Compare May 16, 2015 17:01
@pitr-ch pitr-ch force-pushed the synchronization branch from 0bdff29 to 629d07a Compare May 17, 2015 18:38
@pitr-ch
Copy link
Member Author

pitr-ch commented May 20, 2015

Sorry i know this is blocking pre release, but I still did not figure out why it is failing on 2.0. Please wait. I would also like to get the actor updates merged to edge for the pre release although I know it may not be that important. update: and I still need to investigate the BiasedCondition if it should go to 0.9 too.

@jdantonio
Copy link
Member

We can wait. I greatly appreciate the work you are doing. Can we aim for Monday for the first pre-release, or is that not enough time?

@pitr-ch
Copy link
Member Author

pitr-ch commented May 21, 2015

I'll have some time on weekend so I would like it to be ready on Monday.

Petr Chalupa and others added 2 commits May 23, 2015 10:54
Merge pull request #303 from pitr-ch/devel
f8dcc9f 
Making new Future/Promises lock-free; usage examples
@pitr-ch
Edge - Future tweaks
2be2f43 
- remove all synchronization from path when there is no thread waiting
- improve AllPromise iterations
- zipped futures apply as multiple arguments to block not as a single array
@pitr-ch
Copy link
Member Author

pitr-ch commented May 23, 2015

@jdantonio so it looks like that the cause is kill_rogue_threads :/ I cannot easily remove it, what do we do? update: After removing test are working, but there is still lot of errors on debug level, most probably coming from killing in the timer_task_spec.rb. The kill is always troublesome, I think we are lucky that it breaks only on 2.0.

@jdantonio
Copy link
Member

At LambdaConf with only my phone so I can't easily look at the code or test output, so I'm just throwing out ideas. Have you tried running the test suite without the "rogue threads" hack? I added that very early on because the test suite was weak. We've made a lot of improvements since then. We may be able to remove it.

If the issue is with the rest and not the code you can disable the individual test(s) on Travis. I added an RSpec tag for this. It's used in the Exchanger tests.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 24, 2015

We had a race commenting, I was updating my comment with more information. I've tries not whole suite but just the failing file and it's fine without it, see #296 (comment), but there is still issue. I think best might be to remove the kill_rogue_threads completely. I'll probably postpone the pre-release further (fine by me).

@jdantonio jdantonio mentioned this pull request May 24, 2015
Closed
@jdantonio
Copy link
Member

@pitr-ch Fixed.

It turns out that the problem was with the reset_gem_configuration helper and spec/concurrent/configuration_spec.rb. Once I fixed that I was able to completely remove the kill_rogue_threads function`.

Because I had to make changes to changes to spec_helper.rb this PR cannot automatically be merged with PR #302. I rebased on a different branch (PR #306) and the tests passed on everything but Rbx. For some reason Rbx hangs when running the Actor tests. I cannot reproduce that locally--tests pass for me on all platforms.

I believe we should merge this PR. I'll leave it to you to decide whether to merge or rebase.

PS:

With respect to kill_rogue_threads: I had created that function early on when I was spawning new threads in all the tests. The number of threads in the system would climb by hundreds because they weren't effectively being garbage collected. Now that we are using thread pools in the tests (and our thread pools are very stable) this doesn't appear to be a problem anymore.

@jdantonio jdantonio mentioned this pull request May 24, 2015
Merged
@pitr-ch
Copy link
Member Author

pitr-ch commented May 24, 2015

Great, Thanks! I thought this would be much more work. I'll merge master here, I've already based another PR on this branch so I cannot easily rebase. Closing #306 I'll retest here.

@jdantonio
Copy link
Member

I thought it was going to be a lot more work, too. I honestly thought it would be the bulk of my work between 0.9 and 1.0. I'm very happy it ended up being so much easier!

pitr-ch added 2 commits May 24, 2015 20:48
@pitr-ch
Merge branch 'master' into synchronization
0cfc01c 
* upstream/master: (22 commits)
  Async now hides :new and provides safe :create factory.
  ...
@pitr-ch
Do not run configuration spec on travis
1fd2965
@pitr-ch pitr-ch force-pushed the synchronization branch from 33f838c to 1fd2965 Compare May 24, 2015 19:32
@jdantonio jdantonio mentioned this pull request May 24, 2015
Closed
@pitr-ch pitr-ch mentioned this pull request May 24, 2015
Merged
@jdantonio
Copy link
Member

I know how to fix the tests that are failing on ruby-head. I will fix those after the merge.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 24, 2015

Cool, the rbx seems happy without reconfiguration specs. So I think we are almost ready to merge, I'll just recheck the PR. update: I'll finish that up tomorrow.

@jdantonio
Copy link
Member

Cool. Once you've merged this I'll rebase and merge #307 and #308. After that I'll create pre-release builds of all three gems, publish them, and return to work on #300 and #304.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 25, 2015

There is only one failure https://travis-ci.org/ruby-concurrency/concurrent-ruby/jobs/63966616#L4196 otherwise it's ready to merge. @jdantonio I'll leave the decision up to you, if you want to merge it to unblock rest of the work and fix later or fix first.

@jdantonio
Copy link
Member

That failing test ScheduledTask test is known to be buggy. It's on my list. I'm comfortable merging.

pitr-ch pushed a commit that referenced this pull request May 25, 2015
Merge pull request #296 from ruby-concurrency/synchronization
983db3c 
Synchronization 3.update
@pitr-ch pitr-ch merged commit 983db3c into master May 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Milestone
0.9.0
Development

Successfully merging this pull request may close these issues.
2 participants
@pitr-ch @jdantonio