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

[FEA] Delete thread_safe_resource_adaptor: it is untested and likely no longer used #866

Closed
harrism opened this issue Sep 8, 2021 · 5 comments
Labels
feature request New feature or request inactive-30d

Comments

@harrism
Copy link
Member

harrism commented Sep 8, 2021

Is your feature request related to a problem? Please describe.
thread_safe_resource_adaptor used to be used for pool_memory_resource to make it thread safe, but now this and other MRs are internally thread safe.

Describe the solution you'd like
Delete this adaptor, as it is untested.

Describe alternatives you've considered
Write tests, which is more work.

@harrism harrism added the feature request New feature or request label Sep 8, 2021
@jrhemstad
Copy link
Contributor

I'm not so sure. Talking with @abellina, we ran into a situation where they wanted to use limiting_resource_adapator, but it's not actually thread safe. The thread_safe_resource_adaptor would be a trivial solution.

@jrhemstad
Copy link
Contributor

As far as testing goes, I think it's sufficient to just exercise that the allocation/deallocation work as expected. Trying to test thread safety is usually folly.

@harrism
Copy link
Member Author

harrism commented Sep 17, 2021

limiting_resource_adaptor is now thread-safe. #869

@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@harrism
Copy link
Member Author

harrism commented Feb 1, 2022

Since thread_safe_resource_adaptor is now tested, I'm closing this.

@harrism harrism closed this as completed Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request inactive-30d
Projects
None yet
Development

No branches or pull requests

2 participants