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

PooledArray is potentially not thread safe #55

Closed
bkamins opened this issue Feb 4, 2021 · 1 comment · Fixed by #56
Closed

PooledArray is potentially not thread safe #55

bkamins opened this issue Feb 4, 2021 · 1 comment · Fixed by #56

Comments

@bkamins
Copy link
Member

bkamins commented Feb 4, 2021

See the following example:

julia> Threads.nthreads()
8

julia> using PooledArrays

julia> x = PooledArray(zeros(10^6));

julia> Threads.@threads for i in 1:10^6
           x[i] = i
       end
ERROR: TaskFailedException:
BoundsError: attempt to access 228263-element Array{Float64,1} at index [13310]

I think we should make setindex! thread safe. Any opinions?

@nalimilan
Copy link
Member

Like in CategoricalArrays, the problem is that even if we took a lock on each setindex! call, calling getindex in parallel wouldn't be thread-safe if a new level is added (as the pool may be resized). Yet taking a lock on getindex would kill performance.

Also, taking a lock on each setindex! call would make it impossible to run it in parallel, yet that's perfectly safe if you know that you don't add many levels. Not sure whether it's a typical case or not, but that happens e.g. if you reorder values in the array (e.g. sorting).

What we can do without affecting performance is take a lock when adding new levels. That should at least fix the example you give, and adding levels is relatively slow and can't be done in parallel anyway. But that doesn't provide a lot of safety either.

@bkamins bkamins linked a pull request Feb 20, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants