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

Upgrade Celluloid, remove defer usage #919

Closed
mperham opened this issue May 11, 2013 · 3 comments
Closed

Upgrade Celluloid, remove defer usage #919

mperham opened this issue May 11, 2013 · 3 comments

Comments

@mperham
Copy link
Collaborator

mperham commented May 11, 2013

Use of defer was necessary when Celluloid message dispatch was done within fibers. This requires twice the number of threads. Remove it now that thread dispatch is the default.

@tarcieri
Copy link

Note that Celluloid::TaskThreads aren't actually the default. You'll want to do:

task_class TaskThread

...inside of processor to get this behavior. That said, TaskThreads are actually tested now :O

@mperham
Copy link
Collaborator Author

mperham commented May 12, 2013

Good to know. Using an actual constant makes it hard to stub out Celluloid in testing. I'd prefer a symbol, e.g. dispatch_with :thread

Do I lose anything using threads rather than fibers? Will signals still work?

@tarcieri
Copy link

We can add a symbol as a shortcut.

Threads are a bit trickier to for us to manage and I think there may be some issues with shutdown that remain in 0.14 (specifically that the thread pool isn't correctly reaped.

In theory they're also more heavyweight, but it seems like the 4kB stack size is a far more pressing issue, and with defer you're using both anyway.

gshutler added a commit to gshutler/shoryuken that referenced this issue Jun 4, 2015
Following [sidekiq's lead][1] this is no longer necessary. It also means
that `Shoryuken::Shutdown` will be raised on the worker thread which, in
turn means it is possible to create middleware that respond to it.

 [1]: sidekiq/sidekiq#919
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

2 participants