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
RubyThreadLocalVar: rely on GIL on MRI to avoid problems with thread/mutex/queue in finalizers #856
Conversation
…mutex/queue in finalizers
Could you have reproduced it ? |
No, I do not have a reproducer. I will have to look up the people who had the original problem and ask them to try it. |
I can confirm this patch avoids the segfault I was seeing. I was using ruby version:
Thank you so much @pitr-ch ! Example seg fault without this patch. With the patch there is no seg fault
|
This commit addresses some bit rot that has happened in ace. 1. The concurrent-ruby 1.1.6 release triggers a seg fault in MRI ruby 2.5. There is a PR to concurrent ruby ruby-concurrency/concurrent-ruby#856 which I have verified fixes the seg fault but has not been merged/released. For now I pinned to 1.1.5 which does not have the issue. 2. The puppet 6.14.0 release included some changes that were incompatable with ace. First is the code loading for subclassing `Puppet::Configurer`. This was addressed in `ace` by simply loading all of puppet before `puppet/configurer`. The second issue is that previously puppet's logic for figuring out which server to connect to (based on SRV settings, server_list or Puppet[:ca_server] vs Puppet[:server} was spread out all over the place. Puppet switched to using a new http client in 6.14.0 So the logic for resolving which host to connect to is based on a set of resolvers. The change in behavior is that previous ace was pushing `:server` onto the context and puppet would sometimes look that value up when making a connection: https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/connection.rb#L31 3. Rake was bumped due to some CVE that did not really affect our project.
@donoghuc thanks for the confirmation! |
Thank you for this patch! I believe I'm encountering this segfault as well. Is there a timeline for merging this PR and releasing? |
@pitr-ch is there anything I can do or testing that I can do to help get this merged? If not, does anyone have a smallish workaround while we wait for this patch to land? Thanks for your time and energy spent on this project! |
@chrisseaton @jdantonio @sh286 thoughts? |
It's @pitr-ch's code - hopefully he'll take a look after this ping or maybe if he's blocked by work he can let me know what needs doing on it. |
I've been unable to get a response from @pitr-ch at all on this. (I hope they are ok) Is there anything else you can do to help me out here @chrisseaton ? |
I'll try to reach him and if he can't I'll get back to you and do something. |
Sorry for letting you wait. I've been busy and had to put this aside. Merging. |
@pitr-ch Thanks so much! you rock |
No worries! Just glad to see this get merged. I'm glad to see you are alright and concurrent-ruby still has active maintainers 👍 |
Awesome, thank you! |
@pitr-ch Is there a release schedule posted somewhere or do you have any idea when we cut a new release? Let me know if there's any testing or help I can contribute. |
No schedule, I should be able to find some time to do it this week. |
Released 🎉 |
concurrent-ruby was locked to 1.1.5 in puppetlabs#69 due to an issue in 1.1.6. A fix for that was released on concurrent-ruby-1.1.7: ruby-concurrency/concurrent-ruby#856 Recently, in puppet-runtime, concurrent-ruby was bumped to 1.1.8 https://github.com/puppetlabs/puppet-runtime/pull/446/files#diff-6393a204a090900f17b88f58f21f2b5c355271b5e8df85328a80accbc2c503e0R2 making ruby pe-ace-server unable to start: ``` -- Unit pe-ace-server.service has begun starting up. May 17 08:15:28 main-wisdom puma[15711]: /opt/puppetlabs/puppet/lib/ruby/2.5.0/rubygems/dependency.rb:312:in `to_specs’: Could not find ‘concurrent-ruby’ (= 1.1.5) - did find: [concurrent-ruby-1.1.8] (Gem::MissingSpecVersionError) ``` This commit removes the lock of concurrent-ruby as the upstream issue was fixed and release.
fix #849
related to #808