Uploaded image for project: 'Core Server'
  1. Core Server
  2. SERVER-55949

Relaxed load in CancellationState::isCanceled can allow for inconsistent state to be read

    XMLWordPrintable

    Details

    • Backwards Compatibility:
      Fully Compatible
    • Operating System:
      ALL
    • Backport Requested:
      v5.0
    • Sprint:
      Service Arch 2021-05-17, Service Arch 2021-06-28
    • Story Points:
      1

      Description

      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. 

        Attachments

          Activity

            People

            Assignee:
            alex.li Alex Li (Inactive)
            Reporter:
            george.wangensteen George Wangensteen
            Participants:
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: