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.

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

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

Closed
NHDaly opened this issue May 22, 2021 · 3 comments · Fixed by #344
Closed

Comments

@NHDaly
Copy link
Contributor

NHDaly commented May 22, 2021

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:

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

function TimeZone(str::AbstractString, mask::Class=Class(:DEFAULT))
# Note: If the class `mask` does not match the time zone we'll still load the
# information into the cache to ensure the result is consistent.
tz, class = get!(TIME_ZONE_CACHE, str) do
tz_path = joinpath(TZData.COMPILED_DIR, split(str, "/")...)

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
Copy link
Contributor

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

@omus
Copy link
Member

omus commented May 26, 2021

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
Copy link
Contributor Author

NHDaly commented Jun 1, 2021

K, this is definitely not done, but i started a PR here: #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. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants