Potential fix for race condition exposed by Node #5625
Conversation
|
I have run the test that was previously failing several hundred times locally with this change, and it has not yet failed. The previous failure rate was around 1/20. |
| holder->creation_phase = GRPC_SUBCHANNEL_CALL_HOLDER_NOT_CREATING; | ||
| if (holder->connected_subchannel == NULL) { | ||
| fail_locked(exec_ctx, holder); | ||
| } else { | ||
| gpr_atm_rel_store( |
There was a problem hiding this comment.
So, there's a similar rel_store in line 156 above. Is that one not dangerous also?
There was a problem hiding this comment.
The answer to this question is that by the time we reach line 156 we've already checked the value of call and determined that it is definitely NULL. We've already separately handled the CANCELED_CALL case and all other values. Additionally, all modifications to this value are under the lock. Arguably, then, all CAS'es should be gone and replaced with load/store pairs (though we do still need an atomic load for when it is checked outside a lock). That is a TODO for later.
Potential fix for race condition exposed by Node
No description provided.