Bug #20606: Thread#thread_variable_get, Thread#thread_variable? and Thread#[] methods handle non-String/Symbol parameter values differently

• Status: Closed
• Assignee: -
• Target version: -
• ruby -v: 3.2.4
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN

[andrykonchin (Andrew Konchin)]

The Thread#thread_variable_get, Thread#thread_variable? and Thread#[] methods handle the key parameter that is not a String or a Symbol in different way but I would expect them to be consistent and raise an exception.

When no thread-local variables were assigned to a thread the Thread#thread_variable_get and Thread#thread_variable? methods don't raise TypeError when argument is of incorrect type. But Thread#[] does raise TypeError exception:

t = Thread.new {}.join

puts t.thread_variable_get(123).inspect # nil
puts t.thread_variable?(123).inspect # false

t[123] # `[]': 123 is not a symbol nor a string (TypeError)

[andrykonchin (Andrew Konchin)]

• Description updated (diff)

[jeremyevans0 (Jeremy Evans)]

This change would be simple to implement, and I agree the fiber/thread variable inconsistency seems odd. There are specs for the current behavior. However, I checked and the specs appear to be added by you only a couple days ago. It seems odd to me to add specs for something you think is a bug. I think specs and tests should attempt to reflect desired behavior, not attempt to exhaustively describe current behavior. Personally, I get annoyed when I am fixing a bug and have to update specs that attempt to document current behavior without considering whether behavior is desired (this happens way more often with specs than with tests). It is better to leave behavior unspecified/untested than to add specs/tests for behavior that may not be desired.

I'm not opposed to changing the behavior, but raising an exception when one was not previously raised presents backwards compatibility issues. We would probably have to warn in 3.4 and not raise until 3.5 (or later). Please add this to the developer meeting agenda if you would like the behavior to change.

[Eregon (Benoit Daloze)]

@jeremyevans0 (Jeremy Evans) The specs were added in https://github.com/ruby/spec/pull/1127 (so not written by Andrii, but reviewed by him).
Adding specs is how we discover this kind of strange behavior.
But it's not always clear if intentional or not, and unfortunately for compatibility other Ruby implementations might even need to be bug-compatible with CRuby in some cases (because some gems might rely on the weird/buggy behavior, or CRuby considers it too incompatible to fix some weird behavior).

In general it's encouraged to file an issue here if the behavior looks weird and use ruby_bug with the expected behavior, if it's clear what that is, and there is a good chance CRuby will fix it.
This was done later in this case, while improving the specs in a truffleruby PR and discovering the unexpected behavior.
BTW I don't even see in the original PR a spec covering the behavior of this issue (only #20607).

IOW please do not be annoyed by this, ruby/spec is trying to test Ruby better and sometimes the behavior is weird so it's spec'd as current behavior in CRuby, and discussed here, and maybe it's fixed in CRuby, and of course tests/specs should be updated to avoid any regression and test the fix.
So the tests/specs should be updated anyway.
I think this situation is unavoidable to some extent, since CRuby is the reference implementation and its behavior very much dictates the specs, and we need to spec as much as possible to ensure compatibility between Ruby implementations.
There are so many edge cases in CRuby, it is often hard to judge if intentional or not, and whether third-party code might rely on the current behavior.

I also understand why you might be annoyed by this, ruby/spec reviewers try to catch such cases early but nobody is perfect, and I think we should be thankful for more tests/specs, even if that means a bit more effort updating them when fixing a bug/strange behavior.

[Eregon (Benoit Daloze)]

I'm not opposed to changing the behavior, but raising an exception when one was not previously raised presents backwards compatibility issues. We would probably have to warn in 3.4 and not raise until 3.5 (or later).

I don't think anyone uses thread_variable_get/thread_variable? with non-String non-#to_str, non-Symbol keys (and these 2 methods are very rarely used).
i.e. I think it should be fine to raise an error in 3.4 for this.
It's already an error when using them and there is one thread-local variable set:

> Thread.current.thread_variable_set :a, 1
> Thread.current.thread_variable_get 12
(irb):3:in `thread_variable_get': 12 is not a symbol (TypeError)

(obviously if others feel it deserves a warning, fine by me, but it seems overkill for this)

[jeremyevans0 (Jeremy Evans)]

Eregon (Benoit Daloze) wrote in #note-4:

I'm not opposed to changing the behavior, but raising an exception when one was not previously raised presents backwards compatibility issues. We would probably have to warn in 3.4 and not raise until 3.5 (or later).

I don't think anyone uses thread_variable_get/thread_variable? with non-String non-#to_str, non-Symbol keys (and these 2 methods are very rarely used).
i.e. I think it should be fine to raise an error in 3.4 for this.
It's already an error when using them and there is one thread-local variable set:

> Thread.current.thread_variable_set :a, 1
> Thread.current.thread_variable_get 12
(irb):3:in `thread_variable_get': 12 is not a symbol (TypeError)

(obviously if others feel it deserves a warning, fine by me, but it seems overkill for this)

I agree that since you already get a TypeError when a thread variable is already set, this is a bug and does not require warning or dev meeting review. Apologies that I missed the "When no thread-local variables were assigned to a thread" part in my initial review. Seems odd that specs were added only for the buggy case (no thread variables set), but not added for the correct case (thread variables set). I added a pull request to fix the issue: https://github.com/ruby/ruby/pull/11109

• Status changed from Open to Closed

Applied in changeset commit:git|7f1fe5f091db3b05c3970e7b7a7c602922729642.

---

Raise a TypeError for Thread#thread_variable{?,_get} for non-symbol

Previously, a TypeError was not raised if there were no thread
variables, because the conversion to symbol was done after that
check. Convert to symbol before checking for whether thread
variables are set to make the behavior consistent.

Fixes [Bug #20606]