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

Resolve Celluloid 0.17.0 compatibility. #2438

Merged
merged 4 commits into from Aug 15, 2015

Conversation

Projects
None yet
4 participants
@digitalextremist
Copy link
Contributor

commented Jul 16, 2015

I looked through sidekiq and this one line was the only thing holding back all the tests. Otherwise, everything seems correct as it is.

Closes #2397
Closes #2420

That's happening in that one line, is that the actorsystem is allowed to initialize, which in turn populates the Tree who is complaining in the tests. Then we can set the global testing flag and go about our business, boot etc.

all tests pass with only this change for #2420 -- bring up the actors…
…ystem in 0.17.0-current style before setting the global
@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

It was all colorful and said it was fabulous so I think that means it's good?

@mperham

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2015

The test suite doesn't really exercise celluloid at all. I use the rails app in myapp/ to exercise things manually.

On Jul 15, 2015, at 18:20, digitalextremist notifications@github.com wrote:

It was all colorful and said it was fabulous so I think that means it's good?


Reply to this email directly or view it on GitHub.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2015

Ok, I'll give that a try. I referenced your error report in the issue, now I'll take a look at that.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2015

On an i7 4790k w/ 16gb of RAM, using the Rails app and the suggested 50,000 jobs:

  • With mri-2.2.2 ~1ms

Having trouble getting rbx and jruby to compile byebug. Will follow up with more results when those are usable. Otherwise, all seems well so far.

@seuros

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2015

@digitalextremist byebug don't work with these engines. Maybe you can use their respective debuggers.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2015

@seuros ah, well I will add Gemfile and gemspec conditions where appropriate then, because the gem is a dependency by default, regardless of which engine is in use.

@jonhyman

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2015

@mperham have you done any additional testing with 0.17.0? There are some memory leak fixes in there that we're interested in, so am keen on trying out 0.17.0 in the coming weeks.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 13, 2015

I haven't done any testing; I was waiting for sign off before trying it out. @digitalextremist You ok with this now?

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

I'll retry removing byebug to be able to test jruby and rbx today .. otherwise I did 50,000 jobs with mri-2.2.2 no problem @mperham.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

@mperham results:

jruby-1.7.20 passed without event, like mri-2.2.2

rbx-2.5.8 segfaulted, but when retested with a partial job queue it passed.
Refilled job queue and reran and reproduced segmentation fault.
Reran with partial job queue: segmentation fault.
Reran with even smaller job queue: segmentation fault.
Reran with even smaller even smaller job queue: segmentation fault.
Reran with even smaller even smaller even smaller job queue: finished.

This does not seem related to Celluloid at first glance. Will research and post accordingly.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

Extremely long segmentation fault log:

https://gist.github.com/digitalextremist/bf64b5b340cb09d70a93

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2015

Status? Am I waiting for your sign off still?

On Aug 13, 2015, at 13:03, Donovan Keme notifications@github.com wrote:

Extremely long segmentation fault log:

https://gist.github.com/digitalextremist/bf64b5b340cb09d70a93


Reply to this email directly or view it on GitHub.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2015

@mperham has anyone reported segfaults with rbx? That's the last thing I'm troubleshooting but I suspect it's not Celluloid doing that.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2015

I have customers that use MRI and JRuby, but I don't know of anyone using rubinius so I'm OK with the current state.

On Aug 13, 2015, at 19:15, Donovan Keme notifications@github.com wrote:

@mperham has anyone reported segfaults with rbx? That's the last thing I'm troubleshooting but I suspect it's not Celluloid doing that.


Reply to this email directly or view it on GitHub.

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2015

@mperham isolated this to a thread issue in Rubinius, in associated ticket ( #2489 )

@brixen & team will need to diagnose and patch for complete Rubinius compatibility.

Moving to rbx-2.5.2 introduces non-segfault error and process becomes a zombie.

Until I am shown otherwise, I believe this is not Celluloid related.

So... you are clear to ship it //

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2015

Should the gemspec require 0.17?

@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2015

I got reports of slight trouble with several libraries all requiring Celluloid, and some were pinning a lower release of the same series... possibly because we have 0.17.x.x versioning rather than true semver right now. The goal is to always take the newest of the 0.17 line, in all gems across a bundle. It has seemed finicky, so I've been using >= 0.17 as I do here.

@mperham

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2015

I'm not clear on your comment. Here's what I'm suggesting. Y/n?

--- a/lib/sidekiq/version.rb
+++ b/lib/sidekiq/version.rb
@@ -1,3 +1,3 @@
 module Sidekiq
-  VERSION = "3.4.2"
+  VERSION = "3.5.0"
 end
diff --git a/sidekiq.gemspec b/sidekiq.gemspec
index f000ce0..a7f37c5 100644
--- a/sidekiq.gemspec
+++ b/sidekiq.gemspec
@@ -18,7 +18,7 @@ Gem::Specification.new do |gem|
   gem.add_dependency                  'redis', '~> 3.2', '>= 3.2.1'
   gem.add_dependency                  'redis-namespace', '~> 1.5', '>= 1.5.2'
   gem.add_dependency                  'connection_pool', '~> 2.2', '>= 2.2.0'
-  gem.add_dependency                  'celluloid', '~> 0.16.0'
+  gem.add_dependency                  'celluloid', '~> 0.17.0'
   gem.add_dependency                  'json', '~> 1.0'
@digitalextremist

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2015

@mperham that should be totally fine, too.

mperham added a commit that referenced this pull request Aug 15, 2015

Merge pull request #2438 from abstractive/celluloid-17
Resolve Celluloid 0.17.0 compatibility.

@mperham mperham merged commit 4d32988 into mperham:celluloid-17 Aug 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.