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

Remove uses of shared_ptr::use_count == 1 ? #11345

Open
kgpai opened this issue Oct 24, 2024 · 2 comments
Open

Remove uses of shared_ptr::use_count == 1 ? #11345

kgpai opened this issue Oct 24, 2024 · 2 comments
Labels
bug Something isn't working triage Newly created issue that needs attention.

Comments

@kgpai
Copy link
Contributor

kgpai commented Oct 24, 2024

Bug description

Recently MacOS SDK deprecated use of shared_ptr::unique (see #11318 ). A workaround is to use shared_ptr::use_count() ==1 , which is what was used underneath by unique(). The problem is that use_count() is unreliable and not meant to be used when the shared_ptr is used across threads , see : https://stackoverflow.com/q/77286007

A lot of places use use_count() == 1 to decide if its safe to mutate said shared_ptr and all of these use cases assume that the Vector is not shared across multiple threads or is synchronized by some other means.

System information

N/a

Relevant logs

No response

@kgpai kgpai added bug Something isn't working triage Newly created issue that needs attention. labels Oct 24, 2024
@kgpai
Copy link
Contributor Author

kgpai commented Oct 24, 2024

cc: @kevinwilfong @Yuhta @czentgr

@Yuhta
Copy link
Contributor

Yuhta commented Oct 24, 2024

We assume each thread using vector should keep a copy of shared_ptr. With this assumption hold the current usages are safe.

We should not expect client code to hold this assumption though, so external code outside Velox should be advised to use a mutex to synchronize the multi-threaded usage of vectors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage Newly created issue that needs attention.
Projects
None yet
Development

No branches or pull requests

2 participants