-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
0.16.1? #457
Comments
Yeah, locking to 0.15.2 is definitely the best course of action right now. I'll be releasing timers 4.0.1 soon which should address #3 |
Regarding 1, when I Ctrl-C Sidekiq, it says this as expected but does not actually exit:
When I send QUIT to the process after that, I get this output:
|
Yes, you should definitely be using timers 4.0.1 IMHO. |
@mperham I haven't had time to dig into the Sidekiq INTR problem. Is there a corresponding Sidekiq issue specifically for that, and can we publicize it to get people digging into it? I'd like to get Sidekiq on 0.16. |
There's a number of closed ones. Feel free to open a new one if you'd like.
|
@tarcieri What's the timeline for 0.16.1? Anything that I can help with? |
@ioquatix AFAIK, the biggest problem right now is Sidekiq shutdown |
What is the issue #? |
Per @mperham, sounds like there should be a new one. Backtrace is earlier in this ticket. |
@mperham and @tarcieri - I've done some research on the Sidekiq hang on exit tonight and I wanted to share my results and get your thoughts. I started by following the procedure @mperham outlined in #466 and experimented with various changes to Sidekiq and Celluloid. Conclusions follow. The source of the hang appears to be these two lines - https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/launcher.rb#L51-L52 The hang on exit is occurring on the second line - the wait call. Makes sense. Now if I change this line - https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/manager.rb#L50 to
then the Sidekiq process will exit on a Ctrl-C. That means the underlying problem looks like a race condition to me. With the If I revert line 50 of manager.rb to the Github version and update https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/manager.rb#L205-L208 to be
then the Sidekiq process will exit on a Ctrl-C. This makes sense, because manager.alive? no longer returns true. So we clear the wait, and nothing hangs. I think this change to I'd submit this change as a PR, but I get some odd errors in the console on exit. Specifically, I get a warning that the code attempted to call a dead actor, with stack traces like this:
Do either of you (or anyone else) understand why these warning messages would be generated in this scenario? |
@petergoldstein Awesome investigative work. call a dead actor means that the actor has been shutdown/terminated and you try to send it a method. I believe there are a small subset of methods you can send a dead actor (e.g. |
...is racy, as |
@ioquatix Yep, I got that. My real question is why we're seeing that happen in this circumstance. After sleeping on it I have a couple of thoughts here:
@tarcieri Yep, agreed. That's definitely a possible source of the issue. However I'd like to understand why I always get the stack trace - 100% of the time - which is not what I'd expect if the race condition were so simple. But I don't have a great understanding of Celluloid internals, so it's very possible my intuition is off here. |
@tarcieri If you have any comments on my thoughts above - especially thought 2 - I'd appreciate any insight you'd be able to share on the best way to use Celluloid in this case. |
@petergoldstein I'd probably suggest an immediate fix rather than trying to make massive changes to the way Sidekiq works today. Your suggested fix:
...looks good to me, but I see how it could generate spurious logspam. Perhaps I could switch the loglevel of calls to dead actors to DEBUG rather than WARN, since in the shutdown case they're spurious. |
All of the Celluloid shutdown issues I had to deal with make me want to change Sidekiq to be much more draconian about shutdown. That is, don't bother trying to shut down the Actors nicely: just stop processing jobs on USR1, start the shutdown timer at TERM and |
@tarcieri I agree - I think the quick fix is desirable right now, provided that the logspam doesn't indicate a real problem. That said, I think it would be a good idea to get a sense of exactly how Sidekiq should be using Celluloid, and target any required changes for the next Sidekiq major version. Sidekiq is awesome, and I use it in most of my Ruby projects (Thanks @mperham !), but even after several hours of digging in I don't entirely understand how it interacts with (or is supposed to use) Celluloid I think it would be advantageous on a number of levels for Sidekiq to serve as an example of Celluloid best practices. That might require some time and effort, but I think it would be beneficial for both projects. I'd be happy to contribute to that effort. |
Version 3.2.4 and below have issues when shutting down. There's also mention in the bugreport of a memory leak, but I haven't been able to confirm it in this app. The fix is to add a >=3.2.5 condition in the Gemfile. That is the first version that fixes this issue, or at least works around it. It seems to be some problem with recent Celluloid versions, or a race condition that only manifests in recent Celluloid versions. Version 3.2.5 of Sidekiq fixes version 0.15.2 of Celluloid as a dependency, which seems to make the problem go away for now. I don't directly include Celluloid, so I'm not too worried about staying with older versions as long as Sidekiq works. For more about this bug: celluloid/celluloid#457
I'm not sure how helpful this is but I'm still getting this with celluloid 0.15.2 and sidekiq 3.2.6. 2014-11-19T15:20:23.411Z 24639 TID-ovxvs5yuo WARN: task was terminated |
All good. |
Thanks @mperham |
Sidekiq users have reported several showstopper bugs in 0.16.
I'll probably release 3.2.5 today which locks to 0.15.2. In the meantime, please update this issue if there's a new release which I can smoke test for you.
The text was updated successfully, but these errors were encountered: