Repository navigation
v1.2.0 release notes do not mention that RubyThreadLocalVar class was removed #986
Description
Problem
The v1.1.0 release defined a class called RubyThreadLocalVar (link). That class does not exist as of v1.2.0 (link) - the file in which it was defined was removed in commit 30465396 - but the removal was not mentioned in the v1.2.0 release notes. I'm not sure if this was an oversight or a bug.
Proposed Solution
Either the changelog should be updated to reflect the breaking change, or the RubyThreadLocalVar should be restored.
Activity
eregon commentedon Jan 23, 2023
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
laser commentedon Jan 23, 2023
@eregon - Nice, thanks for addressing this issue. I don't think that our team (initially) recognized the
Rubyprefix as an indication of an implementation detail. SinceThreadLocalVaris the preferred interface to this functionality, we'll use that.Thanks again for your quick reply. I appreciate it!
joshcooper commentedon Jan 23, 2023
Hi just wanted to add that Puppet was relying on
Concurrent::RubyThreadLocalVarfor both MRI and JRuby, becauseJRubyThreadLocalVarleaked 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@9182bc3eregon commentedon Jan 23, 2023
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:
Concurrent::ThreadLocalVardefined?(Concurrent::RubyThreadLocalVar) ? Concurrent::RubyThreadLocalVar : Concurrent::ThreadLocalVar.apparently concurrent-ruby sucks
apparently this is what I need to stop broken concurrent-ruby
ioquatix commentedon Jan 24, 2023
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:
https://github.com/ruby-concurrency/concurrent-ruby/blob/v1.1.10/lib/concurrent-ruby/concurrent/atomic/ruby_thread_local_var.rb#L6-L8
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_constantmore 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_constantis 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:
Concurrent::RubyThreadLocalVaras an alias forConcurrent::ThreadLocalVarbut mark it as deprecated (and do a v1.2.1 release).joshcooper commentedon Jan 24, 2023
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.
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.
That would be awesome and greatly appreciated.
eregon commentedon Jan 25, 2023
FWIW this API was clearly marked internal since 2015 or earlier:
concurrent-ruby/lib/concurrent/atomic/ruby_thread_local_var.rb
Lines 6 to 8 in 5946545
So I don't believe there could be any confusion there whether this was internal or not.
laser commentedon Jan 25, 2023
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.sysutils/puppet6: Fix build with rubygem-concurrent-ruby 1.2.0+
sysutils/puppet7: Fix build with rubygem-concurrent-ruby 1.2.0+
Upgrade puppet to fix regression.
update rails gems (#144)
sysutils/puppet6: Fix build with rubygem-concurrent-ruby 1.2.0+
sysutils/puppet7: Fix build with rubygem-concurrent-ruby 1.2.0+