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

concurrent-ruby deprecated feature #310

Closed
jdantonio opened this issue Oct 1, 2015 · 7 comments
Closed

concurrent-ruby deprecated feature #310

jdantonio opened this issue Oct 1, 2015 · 7 comments

Comments

@jdantonio
Copy link

I just noticed that this project uses concurrent-ruby. I am the creator and lead maintainer of that project. I see in your gemspec that we've deprecated a feature you depend on. I'm sorry that we've caused you problems but I'd love to help you find a solution. I'm even willing to add new features if need be. Is there anything I can do to help?

@dennym
Copy link
Contributor

dennym commented Oct 1, 2015

👍

@jdantonio
Copy link
Author

Concurrent::ReadWriteLock was released in 0.9.0 and the original author made performance optimizations in 0.9.1. If we can solve your deprecation issue you should be able to replace lib/volt/utils/read_write_lock.rb with our version.

@jdantonio
Copy link
Author

AFAICT the deprecated feature was our (buggy and inefficient) timeout method. It was used in the dispatch_in_thread method of lib/volt/tasks/dispatcher.rb. That has been replaced with the timeout method from Ruby's standard library. If that's the case then you should be able to safely upgrade. Rails currently uses 1.0.0.pre3. We will release 1.0 prior to RubyConf (myself and another team member are both speaking).

We may be able to replace that instance of timeout with a c-r Future. The timeout method in Ruby's standard library spawns a new thread, making it very inefficient. If we perform the block on line 117 in a Future the we can put a timeout on the action and the job will post to our global thread pool, which should be more efficient and more stable.

I'll try to create a PR this afternoon for you to look at.

I also notice that you use thread local variables. In c-r 0.9.1 we released our own implementation of Concurrent::ThreadLocalVar. It was written by the same developer who created Concurrent::ReadWriteLock. It is more efficient than the standard library implementation. It's worth considering in the future.

@ryanstout
Copy link
Member

@jdantonio thanks for the help. A PR would be great, let me know if I can help. Yea, timeout is the feature that I was using, and I had the same thoughts about standard timeout spawning new threads. Basically we need a worker pool with a timeout. Thanks for all of the great work on concurrent-ruby.

Also, I'll check out ThreadLocalVar,

@jdantonio
Copy link
Author

@ryanstout I think we can make that work. Possibly simplify the code in the process. All of our high-level abstractions post to global thread pools, and most offer methods with timeout. More importantly, most of our high-level abstractions also allow the specific thread pool for execution to be configured via dependency injection. So instead of posting directly to @worker_pool in the dispatch method we could do something like this:

task = Concurrent::Future.execute(executor: @worker_pool)
task.wait(@worker_timeout)

I know that the specific implementation will be more complicated than that, but this is the basic idea. There are other options, too. We also have actors and agents. I'll work on it and submit another PR once I get something working.

@ryanstout
Copy link
Member

@jdantonio thanks again, I owe you one. Let me know if I can help. Having timeouts without spinning up a new thread would be great.

@jdantonio
Copy link
Author

PRs #311, #312, and #313 submitted.

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

No branches or pull requests

3 participants