-
Type: Bug
-
Resolution: Fixed
-
Priority: Major - P3
-
Affects Version/s: None
-
Component/s: None
-
Fully Compatible
-
ALL
-
v5.0
-
Service Arch 2021-05-17, Service Arch 2021-06-28
-
1
Consider a cancelation hierarchy as follows:
CancelationSource first; CancelationSource second(first.token());
Now imagine some thread, thread A, calls
first.cancel();
Around the same time, another thread B reads the cancellation states:
if (second.isCanceled()) if (first.isCancelled()) else...
Based on my understanding of memory ordering, the following could happen:
When thread A cancels the first source, it marks the cancelation state for that source as "canceled" and then emplaces its cancelation promise. Because the second source is in a hierarchy with the first source, fulfilling this promise causes this continuation to run inline on thread A, which marks the cancelation state for the second source as "canceled."
However, because CancelationState::isCanceled uses loadRelaxed, it is possible that thread B reads second.isCanceled() == true and first.isCanceled == false; even though the stores are ordered first --> second on thread A, the relaxed load on thread B won't force any synchronization with the system. It is permissible in the memory model for thread B to read second.isCanceled without being synchronized with all the things that happened before the second source was cancelled in thread A. (The best explanation of this for me was the last example, "Mixing Memory Models" on this page: https://gcc.gnu.org/wiki/Atomic/GCCMM/AtomicSync)
Tenant migrations and resharding code wants to rely on the ordering of cancelation sources in a hierarchy to be consistently visible to other threads; I think this goal makes sense and we should forbid reading this inconsistent state.
Thanks to max.hirschhorn for finding this and working through the memory model examples with me.
Acceptance Criteria:
isCanceled() uses load instead of loadRelaxed.