Possible data races on global mutable state in Base?

[NHDaly]

Following up from #49746 / #49774, I thought I'd try to spend a few minutes auditing all the other global mutable data structures we have in Base for thread safety. I didn't check every single one, but these are the ones that stood out to me as possibly racy and needing a lock.

There were others, but I believe they are only ever mutated during startup, when loading the sysimg, so I left them out.

Here's the list of questionable items - I would definitely appreciate a second opinion from other devs. Thanks!

• active_project_callbacks
• package_callbacks
• include_callbacks
• loaded_modules - maybe this one can only be mutated by the main / top-level task anyway? But can it be read from other Tasks while the main task is mutating it?
• methodloc_callback
• postoutput_hooks - this one definitely should
• repl_hooks
• toplevel_load
• update_stackframes_callback

Thanks!

CC: @kpamnany / @vchuravy / @timholy / @vtjnash as people to tag according to git blame

[NHDaly]

(I used LookingGlass.jl to do this search, via):

julia> for n in LookingGlass.module_globals_names(Base, constness=:const, mutability=:mutable)
           println("const $n")
       end
const ACTIVE_PROJECT
const ARGS
const BUILD_TRIPLET
const COMPILETIME_PREFERENCES
const DATAROOTDIR
const DEFAULT_LOAD_PATH
const DEPOT_PATH
const DL_LOAD_PATH
const DOCDIR
const ENABLE_PRECOMPILE_WARNINGS
...

[vtjnash]

This is an awesome list! A couple you also missed from my list include:

• all of the logging code (mostly because of warn once handling)
• allocation of coverage map (see comment on fix #42659, move jl_coverage_visit_line to runtime library #42810)
• the gc_num struct in C (and actually, I think there is a comment in the C code about all of the @allocated counters being broken with threads)

Looks like most of those just need a simple lock associated with each.

The toplevel_load flag is conceptually flawed though, as is the new loading_extension global that is similar to it

julia/base/loading.jl

Lines 1618 to 1619 in 5039d8a

	# require always works in Main scope and loads files from node 1
	const toplevel_load = Ref(true)

There was a brief observation recently that those may be semi-equivalent to isempty(package_locks)
bf379e2#diff-ec80afe9f8ff182765c73ed3fc35a103b561f873a52b1dfe1ce135e4e5f9a2cfL1673

[NHDaly]

Excellent! Thanks for the details, and the confirmation. 👍

[NHDaly]

Looks like most of those just need a simple lock associated with each.

The toplevel_load flag is conceptually flawed though, as is the new loading_extension global that is similar to it

Yeah, cool. I would advocate for just slapping a lock around each of these as a first pass, and then coming back and fixing any other conceptual / correctness issues with the code in question in a later pass. I don't want us to be hunting down memory corruptions in production again like this again if we can avoid it.

As observed in #49774, these are obviously quite problematic. The failures can be severe, inexplicable, and just infrequent enough to make them difficult to track down.