move package loading behind a lock

[KristofferC]

Ref #336, not sure if it completely fixes the issue.

[codecov]

Codecov Report

Merging #339 (ff11abb) into master (7345261) will decrease coverage by 81.80%.
The diff coverage is 100.00%.

❗ Current head ff11abb differs from pull request most recent head b0abfe1. Consider uploading reports for the commit b0abfe1 to get more accurate results

@@            Coverage Diff             @@
##           master    #339       +/-   ##
==========================================
- Coverage   88.11%   6.31%   -81.81%     
==========================================
  Files          10      10               
  Lines         631     618       -13     
==========================================
- Hits          556      39      -517     
- Misses         75     579      +504     

Impacted Files	Coverage Δ
src/loadsave.jl	30.00% <100.00%> (-68.77%)	⬇️
src/registry_setup.jl	0.00% <0.00%> (-96.40%)	⬇️
src/registry.jl	0.00% <0.00%> (-90.85%)	⬇️
src/query.jl	9.00% <0.00%> (-86.90%)	⬇️
src/types.jl	4.16% <0.00%> (-84.07%)	⬇️
src/error_handling.jl	0.00% <0.00%> (-80.00%)	⬇️
src/mimesave.jl	0.00% <0.00%> (-77.78%)	⬇️
src/deprecated.jl	0.00% <0.00%> (-58.70%)	⬇️
src/FileIO.jl	100.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7345261...b0abfe1. Read the comment docs.

[johnnychen94]

Perhaps this indicates that Base.require itself is not threadsafe, because it appears to use plain Base.Condition, rather than Threads.Condition.

Just curious, could this be fixed in upstream Julia?

[KristofferC]

Maybe, but I don't think that lock is really for concurrency safety but to limit recursive require calls. But I am not sure.

[johnnychen94]

I'll set up the CI and check if it works (tomorrow); I have good reason to believe this is the right fix but we still need to appropriately test it.

[johnnychen94]

I've locally verified that this test 1c9db8f captures the #336 by temporarily reverting 7e457f8, so I'll merge with a patch version bump once CI passes.

[johnnychen94]

It looks like for Julia < v1.3, Threads.SpinLock() is required here. Maybe we should it for all Julia versions?

[johnnychen94]

Some strange behavior here when testing CSVFiles: SystemError: dup: Bad file descriptor. using SpinLock doesn't workaround it. Any ideas?

cc: @davidanthoff

[johnnychen94]

Given that multi-threads are not very well supported before Julia 1.3, I decided to only test this with Julia >= 1.3.

I've locally tested it for all minor versions from 1.3-1.6 on my macOS and Ubuntu. For Julia 1.1 it can be fixed by using SpinLock, but for Julia 1.0 and 1.2, it is still broken anyway.

This is only a partial fix to #336 for Julia >= 1.3.

[davidanthoff]

@johnnychen94 sorry, only catching up right now with notifications. Was the problem with CSVFiles.jl resolved before this was merged? Or is there still something I should look into?

[johnnychen94]

@davidanthoff It was then identified an unrelated issue. Sorry for the spam!