Skip to content

Remove WeakReference list in PredictionEnginePoolPolicy. #4992

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

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Apr 1, 2020

Accesses to this list are not thread-safe because we can be enumerating it on one thread (in Return) while another thread adds to it (in Create).

The list is unnecessary because the PredictionEngine will always be "rooted". Either it was being held in memory by someone who got the PredictionEngine from the pool, or it is being held by the ObjectPool itself. So it won't ever be GC'd, and the WeakReference is not doing anything.

Also, inherit from PooledObjectPolicy instead of IPooledObjectPolicy, so it can take the "fastPolicy" path in DefaultObjectPool. (See dotnet/extensions#318).

Fix #4981

Accesses to this list are not thread-safe because we can be enumerating it on one thread (in Return) while another thread adds to it (in Create).

The list is unnecessary because the PredictionEngine will always be "rooted". Either it was being held in memory by someone who got the PredictionEngine from the pool, or it is being held by the ObjectPool itself. So it won't ever be GC'd, and the WeakReference is not doing anything.

Also, inherit from PooledObjectPolicy instead of IPooledObjectPolicy, so it can take the "fastPolicy" path in DefaultObjectPool. (See dotnet/extensions#318).

Fix dotnet#4981
@eerhardt eerhardt requested a review from antoniovs1029 April 1, 2020 19:08
@eerhardt eerhardt requested a review from a team as a code owner April 1, 2020 19:08
Copy link
Member

@antoniovs1029 antoniovs1029 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@eerhardt eerhardt merged commit 54223a1 into dotnet:master Apr 2, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Mar 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PredictionEnginePool.GetPredictionEngine is not thread safe
2 participants