Make TestLogger thread-safe (introduce a lock)

[NHDaly]

Fixes #54439.

• Lock around concurrent accesses to .logs, .message_limits, and .shouldlog_args
• Copy the vector out of the logger at the end, to shield against dangling Tasks.

Before:

julia> Threads.nthreads()
8

julia> function foo(n)
           @info "Doing foo with n=$n"
           @sync for i=1:n
               Threads.@spawn @info "Iteration $i"
           end
           42
       end
foo (generic function with 1 method)

julia> for _ in 1:1000
           @test_logs (:info,"Doing foo with n=10000") match_mode=:any foo(10_000)
       end
julia+RAI(56155,0x1f5157ac0) malloc: double free for ptr 0x128248000
julia+RAI(56155,0x1f5157ac0) malloc: *** set a breakpoint in malloc_error_break to debug

[56155] signal (6): Abort trap: 6
in expression starting at REPL[8]:1

signal (6) thread (1) __pthread_kill at /usr/lib/system/libsystem_kernel.dylib (unknown line)
Allocations: 54370881 (Pool: 54363911; Big: 6970); GC: 119
[1]    56154 abort      julia -tauto

After:

julia> Threads.nthreads()
8

julia> function foo(n)
           @info "Doing foo with n=$n"
           @sync for i=1:n
               Threads.@spawn @info "Iteration $i"
           end
           42
       end
foo (generic function with 1 method)

julia> for _ in 1:1000
           @test_logs (:info,"Doing foo with n=10000") match_mode=:any foo(10_000)
       end

(no crash) :)

[charnik]

Thanks @NHDaly !

[NHDaly]

Oh no, I just realized that this field is also mutated in Logging.shouldlog. I've added a lock around it, but i'm not sure that it makes logical sense, since another thread may overwrite the value right after it's set...

@c42f: Is there any chance you remember what this field is used for? I don't see that it's used anywhere. I think this is the only reason the struct is mutable, is so that we can set that from shouldlog, but I don't see why it's being set.

It's tested here:

julia/test/corelogging.jl

Lines 130 to 134 in 449c7a2

	# Test consistency with shouldlog() function arguments
	@test record.level == logger.shouldlog_args[1]
	@test record._module == logger.shouldlog_args[2]
	@test record.group == logger.shouldlog_args[3]
	@test record.id == logger.shouldlog_args[4]

But that test doesn't really explain it either. It seems like it would only be retaining the shouldlog args for the last invocation.... So what should this even mean with multiple threads logging?

Could we remove this field entirely?

[NHDaly]

For now, I have just added a comment that this field is not logically thread-safe, which is no worse than how things were before this PR, while the rest of the data-races are fixed.

So this PR is a strict improvement.

[jishnub]

Should this be backported?

[NHDaly]

Okay, I plan to merge this when tests pass.

As of now, this PR is a strict improvement. Perhaps in the future, if we determine that the shouldlog_args field is unused and could be removed, we could mark the entire struct immutable instead of mutable, which would let us shrink the locked regions. But that is a pretty small optimization at this point and most likely isn't super important.

Thanks!