New test failure in the Zeitwerk test suite

[fxn]

Hi!

The suite of Zeitwerk has this test to check autoloading blocks context switching (autoloading is thread-safe):

test "constant definition is synchronized" do
  files = [["m.rb", <<-EOS]]
    module M
      sleep 0.5

      def self.works?
        true
      end
    end
  EOS
  with_setup(files) do
    t = Thread.new { M }
    assert M.works?
    t.join
  end
end

It has passed regularly for a long time, but it started to fail in a flaky way both in truffleruby and in truffleruby-head. See for example this build, or this build.

Does it ring a bell?

[fxn]

This is a shell script that essentially does the same as the flaky test:

#!/bin/sh

cat <<EOS > m.rb
module M
  sleep 0.5

  def self.works?
    true
  end
end
EOS

ruby -I. <<EOS
autoload :M, "m"
t = Thread.new { M }
p M.works?
EOS

rm m.rb

If it passes, you see "true" printed. Otherwise, M does not respond to works? (NoMethodError) because there was a context switch before the require triggered by autoload returned.

[fxn]

With CRuby, that passes even with sleep 60.

[eregon]

Thanks a lot for the report.
This is an area where the semantics of CRuby seems unclear, e.g. does it make other threads wait until the end of module M or the end of the file until the constant M is "published"? What happens if the autoloaded file creates threads, do they see M?

In short this is not yet implemented in TruffleRuby (IIRC), because of unclear semantics.
Currently there is a lock on who wins to autoload a constant, but currently no lock for "wait some other thread has finished autoloading", since the constant is published as soon as assigned.
Preventing thread switching itself seems difficult on JVM, and would feel like a hack at best + potential for deadlocks.
I guess we should look at what CRuby does here but the autoload logic in CRuby seems particularly unreadable.

Re Zeitwerk CI, could you exclude this test for TruffleRuby and keep running other tests? If it passed before I would think it was just lucky timings.

[eregon]

BTW to improve the reliability of that test I believe you would need a sleep (smaller, e.g. of 0.1) between the Thread.new and assert M.works?, otherwise it's likely the M.works? runs first and then the other thread will see the constant is autoloading and wait, and anyway that thread does not check what is defined on M. For the test to fail it needs to be the Thread.new running first and defining the constant but not yet the method, before the main thread keeps running and call the method.

[fxn]

Problem is, this is a fundamental property of Zeitwerk, which is a fundamental property of CRuby: You won't see a class half-defined due to context switching while autoloading. Meaning, what the test does (of course, a class may be reopened elsewhere later, not in a generic sense).

BTW to improve the reliability of that test I believe you would need a sleep (smaller, e.g. of 0.1) between the Thread.new and assert M.works?

You're right.

[fxn]

Test revised here: fxn/zeitwerk@016404c. Thanks for the comment about that :).

[fxn]

Hey! I have documented this in the README. I believe it is accurate, but let me know if it should word it in a different way :).

[eregon]

That looks accurate, I'll try to fix this issue soon.

[eregon]

Some more details about how CRuby handles this: ruby/ruby#5788
And: (from @ioquatix)

No, it's literally cached, there is an autoload cache
Instead of setting the constant on the module, it's stashed into an autoload save area
once the autoload is done, all the constants are added to the module and all waiting threads are unblocked

[ioquatix]

I'm slowly figuring out how it works, I guess it's not a bad implementation, but I think variable.c could do with a rewrite. lol. ruby/ruby#5898 fyi

[eregon]

FWIW, I found https://bugs.ruby-lang.org/issues/15663#change-77214 where I asked some autoload questions and which makes it somewhat clear it's so complex nobody understands it fully.
I think it shouldn't be too bad to implement in TruffleRuby because we already have a lock per autoload constant.
But potentially deadlocks is an issue to consider, i.e., what if one thread is loading A and that needs B, and another thread is loading B and that needs A? Deadlock or the "isolation" of autoload is broken in that case?