Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
May 22, 2024
Merged

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

merged 1 commit into from
May 22, 2024

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented May 22, 2024 •
edited
Loading

PR Description

Backports JuliaLang#54497 to RAI julia.

Checklist

Requirements for merging:

@NHDaly
Make TestLogger thread-safe (introduce a lock)
8121b6f 
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 This change should apply to Julia v1.10 builds port-to-master This change should apply to all future Julia 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 This change should apply to Julia v1.10 builds port-to-master This change should apply to all future Julia builds labels May 22, 2024
charnik
charnik approved these changes May 22, 2024
View reviewed changes
Copy link

@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
10 checks passed
@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
@NHDaly @Drvi
Make TestLogger thread-safe (introduce a lock) (#152)
30b2ff2 
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
@NHDaly @Drvi
Make TestLogger thread-safe (introduce a lock) (#152)
2d97d29 
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) :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charnik charnik charnik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@NHDaly @charnik