v1.2.0 release notes do not mention that RubyThreadLocalVar class was removed

[laser]

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.

[eregon]

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]

@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!

[joshcooper]

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: https://github.com/puppetlabs/puppet/commit/9182bc3dd2576f409a6d01fb5c08d392670e90a2

[eregon]

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.

[ioquatix]

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_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 for Concurrent::ThreadLocalVar but mark it as deprecated (and do a v1.2.1 release).

[joshcooper]

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.

[eregon]

FWIW this API was clearly marked internal since 2015 or earlier: https://github.com/ruby-concurrency/concurrent-ruby/blob/5946545032a1f0d046c803c46626bbb90d6100a0/lib/concurrent/atomic/ruby_thread_local_var.rb#L6-L8 So I don't believe there could be any confusion there whether this was internal or not.

[laser]

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.