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

Actress post release fixes #100

Merged
merged 13 commits into from Jun 14, 2014
Merged

Actress post release fixes #100

merged 13 commits into from Jun 14, 2014

Conversation

pitr-ch
Copy link
Member

@pitr-ch pitr-ch commented May 27, 2014

No description provided.

@pitr-ch
Copy link
Member Author

pitr-ch commented May 27, 2014

I've fixed the reset duplication and disabled the reset only for actress tests. Weird thing is that running actress tests standalone at least 5 hundred times did not produce single deadlock on my machine on JRuby. When I run it altogether I'll get deadlocks intermittently, I need to find out how one influences another. I managed to get it much more stable with explicit actor termination though.

@jdantonio
Copy link
Member

Thank you for looking into this.

@pitr-ch pitr-ch closed this May 28, 2014
@pitr-ch pitr-ch reopened this May 28, 2014
@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 1, 2014

Finally, I've got it working :) It's ready for review and merge.

@jdantonio
Copy link
Member

Looks great! 🤘

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 3, 2014

Thanks, I'll keep it open few days if anybody else wants to look at the changes, then I'll merge it.
@jdantonio I'll also add more documentation here or in another PR, what would you thing about releasing 0.6.1 after that?

@jdantonio
Copy link
Member

Absolutely, we should create a 0.6.1 release once you merge this.

@jdantonio
Copy link
Member

@pitr-ch To ease the transition, what would you think about an additional configuration option:

class Configuration
  def use_new_actor
    Concurrent.send(:remove_const, 'Actor')
    Concurrent.const_set(:Actor, Concurrent::Actress)
  end
end

# later...

Concurrent.configure{|config| config.use_new_actor }

class FooActor
  include Concurrent::Actor
end

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 3, 2014

@jdantonio I am not sure, it may create unpleasant surprises and it will keep printing the classes as Actress anyway.

[1] pry(main)> A = Class.new
=> A
[2] pry(main)> B = A
=> A
[5] pry(main)> B
=> A

We can also consider:

  • other name like Actor2
  • do not require any of the actor libs and let an user choose a deprecated or the new one by explicit requiting

Personally I would wait to 0.7 and keep Actress around.

I'll add the documentation here then I'll merge and we can release new version.

@jdantonio
Copy link
Member

Let's wait, then. Safe is more important than convenient.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 5, 2014

I've found one intermittent failing test so one todo for me:

  • fix it.

Other than that this PR is ready to merge. You can peruse documentation here before it's merged. I'll be glad for feedback.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.83%) when pulling dc3bf17 on actress into e4dd3de on master.

@jdantonio
Copy link
Member

@pitr-ch It's your call. I'm comfortable with the work is done. Please fell free to merge whenever you are ready.

@coveralls
Copy link

Coverage Status

Coverage increased (+3.0%) when pulling c0ce8b6 on actress into 49417cb on master.

@pitr-ch
Copy link
Member Author

pitr-ch commented Jun 14, 2014

@jdantonio This branch is ready for merge. Please give the changes in documentation a look before merging if you are ok with them. Links for API doc are changed to go to http://ruby-concurrency.github.io/concurrent-ruby/frames.html.

jdantonio added a commit that referenced this pull request Jun 14, 2014
@jdantonio jdantonio merged commit 46e7bb0 into master Jun 14, 2014
@reference = Reference.new self
@name = (Type! opts.fetch(:name), String, Symbol).to_s
@terminated = Event.new
@executor = Type! opts.fetch(:executor, Concurrent.configuration.global_task_pool), Executor
Copy link
Member

Choose a reason for hiding this comment

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

@pitr-ch I just discovered that this line causes the global_task_pool Delay to evaluate. This makes it impossible to explicitly configure the global task pool. This code will fail with Concurrent::ConfigurationError: global task pool was already set:

require 'concurrent'

Concurrent.configure do |config|
  config.global_task_pool = Concurrent::CachedThreadPool.new
end

I'm not sure if there is anything that can be done about this, but could you please look into it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah that's because ROOT actor is always created when actress is loaded. I'll delay it as well.

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. enhancement Adding features, adding tests, improving documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants