Skip to content

Backport: Make TestLogger thread-safe (introduce a lock) (#54497)#152

Merged
NHDaly merged 1 commit intov1.10.2+RAIfrom
RAI-backport-54497
May 22, 2024
Merged

Backport: Make TestLogger thread-safe (introduce a lock) (#54497)#152
NHDaly merged 1 commit intov1.10.2+RAIfrom
RAI-backport-54497

Conversation

@NHDaly
Copy link
Copy Markdown
Member

@NHDaly NHDaly commented May 22, 2024

PR Description

Backports JuliaLang#54497 to RAI julia.

Checklist

Requirements for merging:

Fixes JuliaLang#54439.

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

Before:
```julia
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
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) :)
@github-actions github-actions bot added port-to-v1.10 port-to-v1.12 This change should apply to Julia v1.12 builds labels May 22, 2024
@NHDaly NHDaly changed the title Backport PR 54497 Backport: Make TestLogger thread-safe (introduce a lock) (#54497) May 22, 2024
@NHDaly NHDaly removed port-to-v1.10 port-to-v1.12 This change should apply to Julia v1.12 builds labels May 22, 2024
Copy link
Copy Markdown

@charnik charnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks @NHDaly !

Yeah, agreed on the meaninglessness of shouldlog_args.

@NHDaly NHDaly merged commit e8cd7a4 into v1.10.2+RAI May 22, 2024
@NHDaly NHDaly deleted the RAI-backport-54497 branch May 22, 2024 18:25
Drvi pushed a commit that referenced this pull request Jun 7, 2024
Fixes JuliaLang#54439.

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

Before:
```julia
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
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) :)
Drvi pushed a commit that referenced this pull request Jun 7, 2024
Fixes JuliaLang#54439.

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

Before:
```julia
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
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) :)
github-actions bot pushed a commit that referenced this pull request Apr 4, 2026
…Lang#59891)

Stdlib: Distributed
URL: https://github.com/JuliaLang/Distributed.jl
Stdlib branch: master
Julia branch: master
Old commit: 3679026
New commit: 661112e
Julia version: 1.13.0-DEV
Distributed version: 1.11.0(Does not match)
Bump invoked by: @IanButterworth
Powered by:
[BumpStdlibs.jl](https://github.com/JuliaLang/BumpStdlibs.jl)

Diff:
JuliaLang/Distributed.jl@3679026...661112e

```
$ git log --oneline 3679026..661112e
661112e Merge pull request #152 from JuliaLang/dependabot/github_actions/actions/checkout-5
5851d0b Bump actions/checkout from 4 to 5
e05b0e9 Merge pull request #148 from MilesCranmer/patch-1
f31344b Display codecov
9390857 CI: Run CI on all PRs, even if the base (target) branch is not `master` or `release-*` (#144)
```

Co-authored-by: IanButterworth <1694067+IanButterworth@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants