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

JsPool recycle is not thread safe #36

Closed
benokit opened this Issue Dec 13, 2018 · 6 comments

Comments

Projects
None yet
2 participants
@benokit
Copy link
Contributor

benokit commented Dec 13, 2018

JsPool.Recycle() method is not thread safe because DisposeAllEngines() is not thread safe:
It clears the _availableEngines first and _metadata next. But in a concurrent environment it can happen that _availableEngines will not be empty when _metadata gets cleared. Consequently, when trying to get an engine whose metadata was cleared we will get an error:

{"Message":"The given key was not present in the dictionary.","Code":null,"StackTrace":" at
System.Collections.Concurrent.ConcurrentDictionary2.get_Item(TKey key) at JSPool.JsPool2.TakeEngine(TPooled engine) at JSPool.JsPool2.GetEngine(Nullable1 timeout)

Check the simulation of this scenario in a unit test: https://github.com/benokit/JSPool/blob/master/tests/JSPool.Tests/JsPoolRecycleTests.cs

Possible solution:

Remove _metadata entirely. Add EngineMetada property to PooledObject class.

@Daniel15

This comment has been minimized.

Copy link
Owner

Daniel15 commented Dec 13, 2018

Good catch!

The easiest thing is probably to take a lock in that function, although removing _metadata entirely is possible now that we have the PooledObject class (which didn't used to exist). Would you like to submit a pull request to fix this?

@benokit

This comment has been minimized.

Copy link
Contributor

benokit commented Dec 13, 2018

Thanks for the quick response. I will prepare a pull request.

@benokit

This comment has been minimized.

Copy link
Contributor

benokit commented Dec 14, 2018

@Daniel15, here is the pool request: #37.

I didn't go with the "lock" solution. It is not that simple. You would have to use the same lock object for three branches of execution: recyle, create engine and return engine, which means that "create engine" and "return engine" would undesirably lock each other. Moreover, because _availableEngines is protected you don't have control of how it will be (ab)used in the derived class.

@Daniel15

This comment has been minimized.

Copy link
Owner

Daniel15 commented Dec 16, 2018

Thank you for the pull request! :)

@Daniel15 Daniel15 closed this Dec 16, 2018

@benokit

This comment has been minimized.

Copy link
Contributor

benokit commented Dec 17, 2018

Thank you! Looking forward for the new release.

@Daniel15

This comment has been minimized.

Copy link
Owner

Daniel15 commented Dec 21, 2018

I'm going to be on vacation soon, so I'll likely release this in the new year.

@Daniel15 Daniel15 reopened this Dec 21, 2018

@Daniel15 Daniel15 closed this Dec 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment