Make excluded threadunsafe tests threadsafe

[IanButterworth]

On JuliaLang/julia#57087 this makes tests pass locally for me

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.24%. Comparing base (3b9e4fd) to head (8b64123).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #121   +/-   ##
=======================================
  Coverage   79.24%   79.24%           
=======================================
  Files          10       10           
  Lines        1913     1913           
=======================================
  Hits         1516     1516           
  Misses        397      397           

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

[JamesWrigley]

I don't think this is safe to merge without #101.

[IanButterworth]

I could test #101 in JuliaLang/julia#57087 ?

[JamesWrigley]

It's already been tested quite extensively 😅 What we're missing is an audit and review: #101 (comment)

That being said... nothing in Distributed uses threadpools, so if JuliaLang/julia#57087 only adds to the interactive threadpool then maybe we're safe. Does @async or Threads.@spawn pay any attention to what threadpool the task is being launched from? If not then it might be enough to change include_thread_unsafe_tests() to only check how large the :default threadpool is.

[IanButterworth]

Yeah it adds an interactive thread but bear in mind that that makes the main thread an interactive thread. A few places in julia tests assumed threadpoolsize() == threadpoolsize(:default)

[IanButterworth]

Does @async or Threads.@spawn pay any attention to what threadpool the task is being launched from?

@async will run on the same thread as the one being launched from.
A bare @spawn will run in the default threadpool irrespective of the one it's called from

julia> Threads.threadpoolsize(:interactive)
1

julia> Threads.threadpoolsize(:default)
1

julia> Threads.threadpool()
:interactive

julia> fetch(Threads.@spawn(Threads.threadpool()))
:default

[JamesWrigley]

Ah ok, then it's definitely not safe to merge without #101 unfortunately. Distributed mostly uses @async but there's also some places where it uses Threads.@spawn so it would end up creating tasks on different threads.

[IanButterworth]

I think you can do @spawn threadpool() foo() to make it run on the same threadpool at least. But not specifically the same thread.

[IanButterworth]

@JamesWrigley should this be merged into #101 ? I'm not sure what the right thing to change it to is though? Just always enable these tests if threadsafety has been fixed?

[JamesWrigley]

Nah no need to merge it into #101 already, we can wait till #101 is merged and then merge to master. And yep, once Distributed is threadsafe then we can just always run those tests.

[JamesWrigley]

The last three commits should be deleted, and then this can be merged.