Such threadsafe, much wow #4

JamesWrigley · 2024-10-29T20:30:25Z

Relanded from: JuliaLang/Distributed.jl#101

As discussed offline this skips the last couple of commits to replace @async with Threads.@spawn, though I did keep f98c4ca since I figured it couldn't hurt.

codecov-commenter · 2024-10-29T20:46:04Z

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.

Project coverage is 86.63%. Comparing base (76df474) to head (c1a3be8).
Report is 38 commits behind head on master.

Files with missing lines	Patch %	Lines
src/cluster.jl	92.30%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master       #4      +/-   ##
==========================================
+ Coverage   86.20%   86.63%   +0.42%     
==========================================
  Files          10       10              
  Lines        1965     1983      +18     
==========================================
+ Hits         1694     1718      +24     
+ Misses        271      265       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jpsamaroo · 2024-10-30T10:20:58Z

src/cluster.jl

+                if jw.state === W_CREATED
+                    lock(jw.c_state) do
+                        wait(jw.c_state)
+                    end
+                end


This seems potentially incorrect; shouldn't the equality comparison be within the lock block?

Ah yes I think you're right, especially since the docstring for c_state says:

c_state::Threads.Condition # wait for state changes, lock for state

Fixed in 9283e6f. But I see that Worker.state is an atomic, should we also be doing atomic reads of that variable everywhere else too? AFAICT most reads are currently non-atomic.

We should probably switch to acquire-release, or keep everything seq-cst for now (reads without @atomic are :monotonic, so we should probably do reads as @atomic :sequentially_consistent jw.state).

Acquire-release is probably more than strong enough for this.

Alrighty, fixed in 8e35ba4. I stuck to seq-cst just because that's the default for @atomic.

jpsamaroo · 2024-10-30T10:25:36Z

test/threads.jl

@@ -0,0 +1,64 @@
+using Test


We need to also enable JULIA_NUM_THREADS in CI.

I don't think we have to because this test sets up its own workers with multiple threads in the exeflags variable. But I also don't have any objection to starting every worker in the tests with multiple threads anyway, maybe it'd be easier to shake out bugs?

I don't think we have to because this test sets up its own workers with multiple threads in the exeflags variable.

Ahh yes, I missed that!

But I also don't have any objection to starting every worker in the tests with multiple threads anyway, maybe it'd be easier to shake out bugs?

It's up to you - I think it could be valuable to try, but maybe we wait for the @async -> Threads.@spawn change, to be able to see what happens when tasks get assigned to other threads.

Yeah we might as well go for it now, added in 8e35ba4.

JamesWrigley · 2024-10-31T22:06:53Z

BTW please don't merge this with the fixup commits, once it's ready to merge I'll rebase and squash them.

JamesWrigley · 2024-10-31T22:22:20Z

Definitely some fishy failures in the latest CI run.

1.11 on OSX outright failed during what looks like the SSH tests. Not sure whether that's a bug in LibSSH.jl or DistributedNext.

Even though all the others passed, on some there's messages like this:

┌ Warning: Failed to gracefully kill worker 3, sending SIGQUIT
└ @ DistributedNext ~/work/DistributedNext.jl/DistributedNext.jl/src/managers.jl:770

And I didn't notice before, but there's some tests that are explicitly disabled when Julia is run with multiple threads.

JamesWrigley · 2024-11-01T13:49:36Z

Can confirm that LibSSH is pretty thread-unsafe, so I put the SSH tests into their own file to be executed with a single thread in a2e7b7a. And I enabled all the other thread-unsafe tests in 02fe4d5.

JamesWrigley · 2024-11-01T14:05:22Z

Now all the SSH tests pass, but:

We still see the Failed to gracefully kill worker messages on nightly.

Windows x86 had a worker failure (which somehow didn't seem to affect the tests):

     Testing Running tests...
      From worker 4:	Unhandled Task ERROR: EOFError: read end of file
      From worker 4:	Stacktrace:
      From worker 4:	 [1] wait
      From worker 4:	   @ .\asyncevent.jl:162 [inlined]
      From worker 4:	 [2] sleep
      From worker 4:	   @ .\asyncevent.jl:272 [inlined]
      From worker 4:	 [3] (::DistributedNext.var"#33#36"{DistributedNext.Worker, Float64})()
      From worker 4:	   @ DistributedNext D:\a\DistributedNext.jl\DistributedNext.jl\src\cluster.jl:198

https://github.com/JuliaParallel/DistributedNext.jl/actions/runs/11630388640/job/32389220599?pr=4#step:6:120

I'm going to refactor the tests into @testsets so it's easier to see exactly where these are coming from.

JamesWrigley · 2024-11-01T21:25:21Z

Put all the tests in @testset's in 8d932fe. That uncovered a couple warnings, one from the topology tests where it was repeatedly overwriting a function with @everywhere, and another in Julia's vendored OffsetArrays code which should be fixed by JuliaLang/julia#56414.

Necessary because LibSSH is not thread-safe.

This makes it much easier to see where errors/warnings are coming from. The tests have been preserved in the exact order they were written, with no changes other than the necessary ones to put them in `@testset`'s (e.g. creating modules in global scope).

JamesWrigley · 2024-11-14T21:33:21Z

No luck with the rmprocs() timeout on nightly yet (#6), but as discussed offline I'll merge this now and revisit it if it's still a problem on 1.12. I think everything else is in a decent state.

Add gitignore

15f6afb

JamesWrigley requested a review from jpsamaroo October 29, 2024 20:30

JamesWrigley self-assigned this Oct 29, 2024

jpsamaroo mentioned this pull request Oct 29, 2024

Roadmap #1

Open

8 tasks

jpsamaroo reviewed Oct 30, 2024

View reviewed changes

JamesWrigley force-pushed the jps/threadsafe_workerstate branch 3 times, most recently from b5dcd9d to 68482e6 Compare November 2, 2024 20:57

JamesWrigley force-pushed the jps/threadsafe_workerstate branch from 2ea7f38 to 880eebe Compare November 14, 2024 20:26

vchuravy and others added 8 commits November 14, 2024 21:37

Make worker state variable threadsafe

853974a

init_multi: Be more thread-safe

a15baf4

Use errormonitor() in a few places

1e4b52f

Update changelog

3c9080d

Always run multi-threaded tests

f6892b5

Move SSH tests into a single-threaded process

da533c1

Necessary because LibSSH is not thread-safe.

Increase threads test wait time to avoid timeouts

c1a3be8

JamesWrigley force-pushed the jps/threadsafe_workerstate branch from 880eebe to c1a3be8 Compare November 14, 2024 20:40

JamesWrigley mentioned this pull request Nov 14, 2024

Removing workers times out on nightly #6

Open

JamesWrigley merged commit b9a8000 into master Nov 14, 2024
11 checks passed

JamesWrigley deleted the jps/threadsafe_workerstate branch November 14, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Such threadsafe, much wow #4

Such threadsafe, much wow #4

JamesWrigley commented Oct 29, 2024

codecov-commenter commented Oct 29, 2024 •

edited

Loading

jpsamaroo Oct 30, 2024

JamesWrigley Oct 30, 2024

jpsamaroo Oct 31, 2024 •

edited

Loading

jpsamaroo Oct 31, 2024

JamesWrigley Oct 31, 2024

jpsamaroo Oct 30, 2024

JamesWrigley Oct 30, 2024

jpsamaroo Oct 31, 2024

JamesWrigley Oct 31, 2024

JamesWrigley commented Oct 31, 2024 •

edited

Loading

JamesWrigley commented Oct 31, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 14, 2024

Such threadsafe, much wow #4

Such threadsafe, much wow #4

Conversation

JamesWrigley commented Oct 29, 2024

codecov-commenter commented Oct 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsamaroo Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamesWrigley commented Oct 31, 2024 • edited Loading

JamesWrigley commented Oct 31, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 1, 2024

JamesWrigley commented Nov 14, 2024

codecov-commenter commented Oct 29, 2024 •

edited

Loading

jpsamaroo Oct 31, 2024 •

edited

Loading

JamesWrigley commented Oct 31, 2024 •

edited

Loading