semian
semian copied to clipboard
Shopify
Possible concurrency issues
@casperisfine @Sirupsen
I was poking around and found what I think are a few (hypothetical) issues when some semian methods are called concurrently. It's possible I'm missing something in the architecture (e.g are adapter methods expected to always be mutexed by the parent driver the way redis is?) but if not then here are some things I noticed:
- multiple concurrent calls to a new adapter can end up racing in
retrieve_or_registercreating multiple instances of the same circuit-breaker and protected resource - various issues in the circuit-breaker, e.g. concurrent calls to
request_allowed?can "lose" successful responses by triggering multiple transitions to the half-open state
are adapter methods expected to always be mutexed by the parent driver the way redis is
Yes. I can't think of any datastore driver that would support being used concurrently from multiple threads.
I am thinking e.g. of mysql, where you have a connection pool; the individual connections are synchronous but the pool itself is concurrent. I was assuming the driver as a whole got a single semian resource, not one per connection?
I was assuming the driver as a whole got a single semian resource, not one per connection?
It's one per connection. MySQL and other relational DB drivers don't handle pooling, you have to handle pooling higher in the stack.
If one driver were to implement pooling, the semian adapter would have to take care of this concern indeed.
OK, adapter mutexing takes care of the first issue (retrieve_or_register) but I believe the circuit-breaker point is still valid since request_allowed? and other such methods are expected to be publicly called outside the scope of the adapter.
request_allowed? and other such methods are expected to be publicly called outside the scope of the adapter
By whom? I can't see a reason to call those methods except to simulate a test example, in which case threading isn't an issue.
Ok, so as discussed offline. request_allowed? can make sense to be called higher on the stack to be able to bail out of a code path early if you know it depends on a datastore.
Because of this it should be thread safe. I'm working on a fix as we speak.
Yeah, it was not designed to be thread_safe (we haven't needed it), but it definitely makes sense for it to be.
👍 to @casperisfine's direction.
