Use Threads.Condition for package locks

[vchuravy]

@Sacha0 reported a concurrency violation in this code.

ERROR: On worker 4:
LoadError: ConcurrencyViolationError("lock must be held")
Stacktrace:
  [1] concurrency_violation
    @ ./condition.jl:8
  [2] assert_havelock
    @ ./condition.jl:25 [inlined]
  [3] assert_havelock
    @ ./condition.jl:48 [inlined]
  [4] assert_havelock
    @ ./condition.jl:72 [inlined]
  [5] notify
    @ ./condition.jl:132
  [6] #notify#548
    @ ./condition.jl:130 [inlined]
  [7] _require
    @ ./loading.jl:1159
  [8] require
    @ ./loading.jl:1013
  [9] require
    @ ./loading.jl:997
 [10] top-level scope
    @ /build/source/usr/share/julia/stdlib/v1.7/Distributed/src/macros.jl:200
 [11] include
    @ ./client.jl:451
 [12] top-level scope
    @ none:1
 [13] eval
    @ ./boot.jl:373
 [14] #115
    @ /build/source/usr/share/julia/stdlib/v1.7/Distributed/src/process_messages.jl:274
 [15] run_work_thunk
    @ /build/source/usr/share/julia/stdlib/v1.7/Distributed/src/process_messages.jl:63
 [16] run_work_thunk
    @ /build/source/usr/share/julia/stdlib/v1.7/Distributed/src/process_messages.jl:72
 [17] #108
    @ ./threadingconstructs.jl:178
in expression starting at /tmp/nix-build-delve-build.drv-0/delve-source/test-

This was on #38405, but I suspect with enough creativity one could come up with a pure Threads.@Spawn example.
@Sacha0 can I punt thinking about a test to you?

[vtjnash]

This lock/wait code pattern is always wrong (it is why it forces you to write this out), since it is failing to protect the data. You'll need to rewrite much more of this code to be thread-safe and data-race-free.

[vchuravy]

Can you expand on that? The usage pattern of Threads.Condition is something that is sparsely documented https://docs.julialang.org/en/v1/base/multi-threading/#Synchronization, so if this is "always" wrong it would be great if you could improve the documentation.

Is your point that I might be woken to soon and I need to ensure that the condition I am waiting for has actually been met?

[vtjnash]

When you take a lock, you need to ensure it contains all of the shared data

[JeffBezanson]

I think we could use a lock for all the global package state, which can be shared by all the per-package Conditions?

[KristofferC]

Since this is marked as a "bugfix". What is the bug here, how can it be triggered? Anything that has changed here compared to earlier Julia versions?

[vchuravy]

Since this is marked as a "bugfix". What is the bug here, how can it be triggered? Anything that has changed here compared to earlier Julia versions?

It's not a regression, but package loading/require is not threadsafe. I can try coming up with a MWE, but the error above stems from a parallel test runner used at RelationalAI.

[vchuravy]

I wont have time to work on this, but I created a reproducer independent of Distributed.jl #41546