xAODRootAccess: Work around root 6.20.02 destructor issue.

~TMethodCall dereferences gCling. But if it is being run as part of global destructors, then gCling may have already been destroyed by the time the TMethodCall destructor runs.

Change TAuxVectorFactory to hold the TMethodCall instance by pointer to guard against this.

Needed to fix test failure in xAODDataSource.

See !31633 (merged).

This merge request affects 1 package:

• Control/xAODRootAccess

Adding @akraszna ,@ssnyder as watchers

added Core master review-pending-level-1 labels

CI Result SUCCESS (hash ea792651)

	Athena	AthSimulation	AnalysisBase	AthGeneration
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 12257]

added urgent label

This looks good to me.

MLB (L1)

added review-approved label and removed review-pending-level-1 label

Functionally it looks fine. Though I can't help but think that a slightly "fancier" solution could've been nicer. I'm thinking along the lines of:

namespace xAOD {
   namespace detail {
      class TMethodCallDeleter {
      public:
         void operator()( void* ptr ) {
            if( ! gCling ) {
               return;
            }
            TMethodCall* p = reinterpret_cast< TMethodCall* >( ptr );
            delete p;
         }
      };
   }
}

And then define m_assign as:

mutable std::unique_ptr< TMethodCall, xAOD::detail::TMethodCallDeleter > m_assign;

I realise that this may be shooting a duck with a cannon, but I do wonder if such a solution would not be better. It would also make it easier to remove the custom deleter in case we may not need it anymore in the future. (Without changing the rest of the code, and still keeping it safe for memory leaks...)

Similar to !31988 (closed) I'm having a very hard time to find the test that this fix is meant for. I don't doubt that it exists, but I can just not find which one it is...

This is marked urgent so can we try and get the discussion resolved today.

@fwinkl would have to say how urgent this really is... I don't believe this code is used in any "important" part of the master code right now. (Like reconstruction, simulation, these sort of things.) So I'm not convinced that this is a matter of life or death to merge in today.

This issue causes failures in these tests:

          4 - xAODDataSource_dataFrameTypeConversion_test_ctest (Failed)
          5 - xAODDataSource_dataFrameElementLink_test_ctest (Failed)

They show up as segfaults as the jobs are exiting.

A discussion for a fancier alternative is still open. Does still stand or do you want this MR to go ahead?

resolved all threads

merged

mentioned in commit fff49a42

added sweep:ignore label