Comments (8)
Hi just wanted to add that Puppet was relying on Concurrent::RubyThreadLocalVar
for both MRI and JRuby, because JRubyThreadLocalVar
leaked a reference to the JRuby instance after it was discarded. I don't know if an issue was filed... but here's the commit where that change was made: puppetlabs/puppet@9182bc3
from concurrent-ruby.
Any class starting with a Ruby, JRuby, Java, TruffleRuby prefix is as fairly obvious an implementation detail.
Relying on that is unsupported.
So it an implementation detail and therefore does not have any compatibility guarantee and is not mentioned in the CHANGELOG.
Were you using it? Why instead of using the supported ThreadLocalVar
?
The list of public classes is in the documentation: https://ruby-concurrency.github.io/concurrent-ruby
from concurrent-ruby.
I can't find an issue about that, would have been nice to file one before relying on a library's internals.
I think you can easily preserve your behavior in Puppet either:
- by fixing the concurrent-ruby version to 1.2+ and using
Concurrent::ThreadLocalVar
- or if you need to support both versions of concurrent-ruby then something like
defined?(Concurrent::RubyThreadLocalVar) ? Concurrent::RubyThreadLocalVar : Concurrent::ThreadLocalVar
.
from concurrent-ruby.
Hi folks,
My two cents, since I'm subscribed to the thread:
I think that it was a mistake for our developer to use RubyThreadLocalVar
. I agree that the documentation calls out that the class was private. We didn't look at that documentation, clearly. That's on us.
I also agree that distinction between public and private in Ruby is hard to enforce. I'm not sure how to solve that problem.
Based on what I've learned in this thread, I no longer think that the v1.2.0 release notes should have mentioned RubyThreadLocalVar
, as it was not part of the library's public interface.
from concurrent-ruby.
FWIW this API was clearly marked internal since 2015 or earlier:
So I don't believe there could be any confusion there whether this was internal or not.
from concurrent-ruby.
@eregon - Nice, thanks for addressing this issue. I don't think that our team (initially) recognized the Ruby
prefix as an indication of an implementation detail. Since ThreadLocalVar
is the preferred interface to this functionality, we'll use that.
Thanks again for your quick reply. I appreciate it!
from concurrent-ruby.
I agree with @eregon, however I think it's even more clear cut than "any class starting with a Ruby implementation name is an implementation detail".
Specifically:
Mentions that the class is private and an implementation detail. If you search the documentation, AFAICT, it doesn't even show up: https://www.rubydoc.info/gems/concurrent-ruby/1.1.10 - this applies to all code which is an implementation detail.
I think it's fair to say, the authors and maintainers of the library have done their best to clearly mark private implementation details. However, I'm left wondering how code has come to depend on RubyThreadLocalVar
. Was it possible in the past this was not marked as an implementation detail?
We should consider using private_constant
more judiciously perhaps.
@laser do you think we should add the removal and changes to implementation details? I'm not against it, but it might detract from the actual list of public interface changes.
Regarding Puppet using an internal implementation detail, well, I can understand the pain, but we are stuck between a rock and a hard place. That interface was never expected to be used publicly. As maintainers, if we don't know about it, we can't do anything about it. If no issue was filed about the leak, or promoting that interface to a public one, I don't see how this situation could have been avoided (I believe private_constant
is a somewhat recent innovation to Ruby).
I see three potential solutions going forward - the one provided by @eregon makes the most sense to me and is the most future looking.
The other two options are:
- Pin your dependency on concurrent-ruby 1.1, or
- Consider re-introducing
Concurrent::RubyThreadLocalVar
as an alias forConcurrent::ThreadLocalVar
but mark it as deprecated (and do a v1.2.1 release).
from concurrent-ruby.
I agree this is a Puppet bug and we're taking steps to remedy. I don't remember the exact specifics of what the underlying issue was, but it doesn't help us now.
That interface was never expected to be used publicly
Yes that's true, but due to Hyrum's law combined with the fact that it's hard to hide implementation details in Ruby, I would have rather seen this change released as version 2.0. I tend to take a conservative approach because I've been bitten by this kind of thing so many times.
re-introducing Concurrent::RubyThreadLocalVar as an alias for Concurrent::ThreadLocalVar
That would be awesome and greatly appreciated.
from concurrent-ruby.
Related Issues (20)
- why use wait_for_termination method will stuck the code HOT 2
- DaemonThreadFactory creating new Java thread factory each time it creates a new thread HOT 8
- `Concurrent::Promises::Future#value!` can return `nil` even when promise is not resolved yet HOT 6
- Version 1.2.2 crashes with Segmentation fault HOT 3
- Fiber.new causes SEGV when using Ruby 3.3.0 on Rails 7.1.2 in M1 Mac Docker environment HOT 5
- Segfault in lock_local_var.rb on aarch64 HOT 3
- [REDIRECT] Segfault on Ruby 3.3.0 on linux-aarch64? See https://bugs.ruby-lang.org/issues/20085
- Add new CI job with RUBY_MN_THREADS=1 HOT 4
- Repeated calls to `ResolvableEvent#resolve` with `raise_on_reassign = true` does not raise
- Semaphore doesn't raise exception when negative int passed to constructor HOT 3
- `Concurrent::Hash` default initialization is not fully thread-safe HOT 8
- Slow require 'concurrent' on windows HOT 3
- Remove Rubinius-related code
- Does not run the method when executed HOT 1
- test failure due to "uninitialized constant Concurrent::CAtomicReference" HOT 7
- CAtomicFixnum should probably be a private constant HOT 1
- support re-raising exceptions when shutting down a timertask HOT 4
- NameError: uninitialized constant Concurrent::RubyThreadLocalVar HOT 1
- The Concurrent::Map default_proc is passed a Concurrent::Hash instead of the Concurrent::Map HOT 8
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from concurrent-ruby.