JuliaTime / TimeZones.jl

IANA time zone database access for the Julia programming language
Other
86 stars 52 forks source link

UndefRefError: Race Condition parsing TimeZone from multiple threads: needs coordination #342

Closed NHDaly closed 3 years ago

NHDaly commented 3 years ago

The TIME_ZONE_CACHE is not currently thread safe, so parsing a TimeZone object via the following constructor is currently a race condition, and can lead to crashes and errors:

https://github.com/JuliaTime/TimeZones.jl/blob/906cea08c2ca4a5c76b12e14fb60d5df7990fbf7/src/types/timezone.jl#L1

https://github.com/JuliaTime/TimeZones.jl/blob/906cea08c2ca4a5c76b12e14fb60d5df7990fbf7/src/types/timezone.jl#L43-L47

The cache either needs to be surrounded by a lock (though this would serialize all TimeZone constructors and probably be worse than having no cache at all), or replicated via thread-local caches.

I would probably suggest a thread-local cache? But interested in other ideas! :)

Maybe just something like this?:

const THREADLOCAL_TIME_ZONE_CACHES = Vector{Dict{String,Tuple{TimeZone,Class}}}()

function TimeZone(str::AbstractString, mask::Class=Class(:DEFAULT))
    tz, class = get!(THREADLOCAL_TIME_ZONE_CACHES[Threads.threadid()], str) do
    end
end

function __init__()
    append!(THREADLOCAL_TIME_ZONE_CACHES, [Dict{String,Tuple{TimeZone,Class}}() for _ in 1:Threads.nthreads()]
end
KristofferC commented 3 years ago

As a note, it is generally better to allocate the object on the thread that it will be used (see https://github.com/JuliaWeb/HTTP.jl/pull/704 and https://github.com/JuliaLang/julia/blob/e4fcdf5b04fd9751ce48b0afc700330475b42443/stdlib/Random/src/RNGs.jl#L369-L385 for examples).

omus commented 3 years ago

Thanks for filing the issue and the suggestions. Feel free to make a PR, otherwise I'll try to get to this during the week.

NHDaly commented 3 years ago

K, this is definitely not done, but i started a PR here: https://github.com/JuliaTime/TimeZones.jl/pull/344

@omus maybe you can pick it up from there? ❤️ Please feel free to edit the PR directly.

🙏 @KristofferC thanks for the examples, that's good to see. 👍