Fix thread-safety problem in GaudiHandle

Fix thread-safety problem in GaudiHandle that was observed to cause a rare crash in Athena.

The component retrieval code in GaudiHandle can lead to operator->() returning a null pointer for a valid handle, if it is called simultaneously from multiple threads.

The relevant code in GaudiHandle is:

  T* operator->() {
    assertObject();
    return m_pObject;
  }


  void assertObject() const {
    if ( UNLIKELY( !isValid() ) ) {
       throw GaudiException ...
    }
  }


  bool isValid() const {
    // not really const, because it may update m_pObject
    return m_pObject || retrieve().isSuccess();
  }


  StatusCode retrieve() const {
    // not really const, because it updates m_pObject
    StatusCode sc = StatusCode::SUCCESS;
    if ( m_pObject && release().isFailure() ) { sc = StatusCode::FAILURE; }
    if ( sc && retrieve( m_pObject ).isFailure() ) {
      m_pObject = nullptr;
      sc        = StatusCode::FAILURE;
    }
    return sc;
  }

Note that if the pointer is already set when retrieve() is called, it clears it and redoes the lookup.

So we could have:

  thread 1 calls operator->.  m_pObject is not set, so it executes
  through to retrieve() after the test on m_pObject.

  thread 2 calls operator->.  m_pObject is not set, so it executes
  through to the start of retrieve().

  thread 1 continues, does the lookup, sets m_pObject and returns.

  thread 2 continues, tests m_pObject, finds it set, and calls reset()
  to clear it.

  thread 1 continues, returning through operator->.  m_pObject is now
  null, so it returns null.

  thread 2 continues with the retrieve, and returns a valid pointer
  from operator->.

I've tentatively tried to fix this by making m_pObject atomic, and changing retrieve() so that it will not change m_pObject if it is already set. I think that's consistent with what's implied by retrieve() being const. ATLAS software seems fine with this change, but this is, however, a potential change of behavior, so i point it out here.

changed title from Fix thread-safety problem in GaudiHandle that was observed to casue to Fix thread-safety problem in GaudiHandle

changed the description

added 1 commit

• 209dd7d6 - forgot to commit one change

Compare with previous version

added 1 commit

• d3684bce - formatting

Compare with previous version

changed milestone to %v33r2

added lhcb-gaudi-head label

assigned to @clemenci

• [2020-06-14 00:08] Validation started with lhcb-gaudi-head#2631
• [2020-06-15 00:17] Validation started with lhcb-gaudi-head#2632
• [2020-06-16 00:12] Validation started with lhcb-gaudi-head#2633
• [2020-06-17 00:07] Validation started with lhcb-gaudi-head#2634
• [2020-06-17 02:45] Validation started with lhcb-gaudi-head#2635
• [2020-06-18 02:11] Validation started with lhcb-gaudi-head#2636

approved this merge request

merged

mentioned in commit f4d7918c