Fix concurrency bug #26
Conversation
|
Sure thing! Attached a small test that fails with the old code. |
There was a problem hiding this comment.
Thank you! Still, I would propose to add some more testing of contract here.
Note: I do think, that this implementation will evolve (by using approach without locks) and also could be used in other providers (on different platforms) so I would be very happy if there will be tests which properly tests the contract!
| } | ||
|
|
||
| @Test | ||
| fun testConcurrentAccessOnCachedPoolShouldNotReuseInstances() = runTest { |
There was a problem hiding this comment.
while testing concurrent access is fine, I would say that we don't really need it here.
We can easily write a sequential test here, something like:
pool.use { i1 -> pool.use { i2 -> pool.use { i3 -> }}}So let's keep both sequential and concurrent variant.
Additionally it would be good to not only count instantiate call, but to check, that instances are different.
So f.e make instance an Int instead of object and save instantiateCount there. And then assert, that the instances used in use are different.
And then, we are good to go!
Good idea! Have you considered borrowing a battle-tested implementation? For example, ktor's Pool looks reasonable to me and supports major platforms (although I didn't notice much in the way of unit testing in that repository). |
|
I added an overlapping sequential unit test, simplified a bit, and added some assertSame/NotSame. |
Yeah, so, most likely I wanted to borrow an idea of pool implementation from okio/ktor/others - I don't want to add additional dependencies for such small thing. |
|
Makes sense to me! Thanks for the quick review. Looking forward to your next release! |
(cherry picked from commit 2837434)
Thank you for this excellent library!
I ran into unexpected runtime exceptions while decrypting concurrently on the JVM. The issue appears to be due to a typo in
pooling.kt(see PR). The proposed change fixes the crashes for my use case.