spotify / voyager

🛰️ An approximate nearest-neighbor search library for Python and Java with a focus on ease of use, simplicity, and deployability.
https://spotify.github.io/voyager/
Apache License 2.0
1.26k stars 51 forks source link

Add lock to avoid segfault while resizing #61

Closed dylanrb123 closed 5 months ago

dylanrb123 commented 5 months ago

There is some unsafe memory access in the resizeIndex method, where it deletes and moves around memory while other threads may be accessing it. This PR adds a read-write lock to ensure that the resizeIndex function has exclusive access to the underlying resources when it needs to move the memory around

dylanrb123 commented 5 months ago

Do we also need to take a shared lock over resizeLock when calling addPoint? (addPoint is reading the data structures that get mutated while calling resizeIndex, and IIRC would also hit a segfault if we call addItems in parallel with resizeIndex.)

addPoint only gets called after resizeIndex returns in the addItems function of TypedIndex so that wouldn't be possible in normal operation, however if someone is manually calling resizeIndex in one thread while calling addItems in another then it could cause trouble. I could take the shared lock just in case but seems pretty unlikely to happen in practice, what do you think?

psobot commented 5 months ago

I think it's still definitely worth locking there too - there are cases where multiple concurrent calls to addItems could still cause a segfault here; i.e.:

Thread A Thread B
Call addItems
Call addItems
Call addPoint
cur_element_count++
Index full, call resizeIndex
data_level0_memory_new = realloc(data_level0_memory_, ...)
*data_level0_memory_ 💥
dylanrb123 commented 5 months ago

👍 sounds good, will try to replicate in test and add the lock