Ensure a delegate built for the old tracing mode doesn't replace the new one (#2118)

Author: BCSharp

src/core/IronPython/Runtime/FunctionCode.cs

@@ -682,15 +682,22 @@ internal void LazyCompileFirstTarget(PythonFunction function) {
         /// <summary>
         ///   Updates the delegate based upon current Python context settings for recursion enforcement and for tracing.
         /// </summary>
+        /// <remarks>
+        ///   With <paramref name="forceCreation"/> false, this is the update done by switching tracing on or off (<see cref="UpdateAllCode"/>)
+        ///   and it always sets the target. With <paramref name="forceCreation"/> true, the update is not synchronized with such a switch,
+        ///   so the delegate is set as the target only if the tracing mode it was built for is still current (see <see cref="PublishTarget"/>).
+        /// </remarks>
         /// <returns>
-        ///   The delegate set as the target, or null if only a lazy target was set (only when <paramref name="forceCreation"/> is false).
-        ///   A caller that needs the delegate should use this result rather than read <see cref="Target"/> again: another thread may change
-        ///   it at any time, e.g. by switching tracing on or off.
+        ///   The delegate for the tracing mode that was current when the update started, or null if only a lazy target was set (only when
+        ///   <paramref name="forceCreation"/> is false). A caller that needs the delegate should use this result rather than read
+        ///   <see cref="Target"/> again: another thread may change it at any time, e.g. by switching tracing on or off.
         /// </returns>
         internal Delegate UpdateDelegate(PythonContext context, bool forceCreation) {
+            // Read only once: the delegate is built for this mode, even if another thread switches tracing on or off meanwhile.
+            bool tracing = context.EnableTracing;
             Delegate finalTarget;
 
-            if (context.EnableTracing && _lambda != null) {
+            if (tracing && _lambda != null) {
                 if (_tracingLambda == null) {
                     if (!forceCreation) {
                         // the user just called sys.settrace(), don't force re-compilation of every method in the system.  Instead
@@ -723,10 +730,44 @@ internal Delegate UpdateDelegate(PythonContext context, bool forceCreation) {
 
             finalTarget = AddRecursionCheck(context, finalTarget);
 
-            SetTarget(finalTarget);
+            if (forceCreation) {
+                PublishTarget(context, tracing, finalTarget);
+            } else {
+                SetTarget(finalTarget);
+            }
             return finalTarget;
         }
 
+
+        /// <summary>
+        ///   Sets the target to a delegate built for the given tracing mode, unless that mode is no longer current.
+        /// </summary>
+        /// <remarks>
+        ///   Used for updates that are not synchronized with switching tracing on or off (<see cref="UpdateAllCode"/>), so that a delegate
+        ///   built for the previous mode cannot replace the target set for the new one. Such a delegate stays in its cache
+        ///   (<see cref="_normalDelegate"/> or <see cref="_tracingDelegate"/>) and is used when the mode switches back.
+        /// </remarks>
+        /// <param name="tracing">
+        ///   The tracing mode that <paramref name="target"/> was built for: true for the tracing delegate, false for the normal one.
+        /// </param>
+        /// <param name="target">
+        ///   The delegate for the target, with the recursion check already added (see <see cref="AddRecursionCheck"/>).
+        /// </param>
+        internal void PublishTarget(PythonContext context, bool tracing, Delegate target) {
+            if (context.EnableTracing != tracing) {
+                return;
+            }
+
+            SetTarget(target);
+            Thread.MemoryBarrier();
+            while (context.EnableTracing != tracing) {
+                tracing = context.EnableTracing;
+                UpdateDelegate(context, forceCreation: false);
+                Thread.MemoryBarrier();
+            }
+        }
+
+
         /// <summary>
         ///   Sets a target that creates the delegate for the current mode (tracing or not) when it is first needed.
         /// </summary>
@@ -751,7 +792,7 @@ private void SetLazyTarget() {
         internal void SetDebugTarget(PythonContext context, Delegate target) {
             _normalDelegate = target;
 
-            SetTarget(AddRecursionCheck(context, target));
+            PublishTarget(context, tracing: false, AddRecursionCheck(context, target));
         }
 
         /// <summary>
@@ -963,11 +1004,11 @@ public TargetUpdaterForCompilation(PythonContext context, FunctionCode code) {
             }
 
             public void SetCompiledTarget(object sender, LightLambdaCompileEventArgs e) {
-                _code.SetTarget(_code.AddRecursionCheck(_context, _code._normalDelegate = e.Compiled));
+                _code.PublishTarget(_context, tracing: false, _code.AddRecursionCheck(_context, _code._normalDelegate = e.Compiled));
             }
 
             public void SetCompiledTargetTracing(object sender, LightLambdaCompileEventArgs e) {
-                _code.SetTarget(_code.AddRecursionCheck(_context, _code._tracingDelegate = e.Compiled));
+                _code.PublishTarget(_context, tracing: true, _code.AddRecursionCheck(_context, _code._tracingDelegate = e.Compiled));
             }
         }
 

src/core/IronPython/Runtime/Operations/PythonOps.cs

@@ -1595,8 +1595,8 @@ static Func<CodeContext, CodeContext> GetClassCode(CodeContext/*!*/ context, Fun
                     return target ?? (Func<CodeContext, CodeContext>)funcCode.UpdateDelegate(context.LanguageContext, true);
                 } else {
                     if (funcCode.Target == null) {
-                        funcCode.SetTarget(body);
                         funcCode._normalDelegate = body;
+                        funcCode.PublishTarget(context.LanguageContext, tracing: false, body);
                     }
                     return body;
                 }

src/core/IronPython/Runtime/PythonContext.cs

@@ -3895,7 +3895,7 @@ internal void SetTrace(object o) {
                 var newCounted = newTraceListener?.IsDummyListener == false;
 
                 if (oldCounted != newCounted) {
-                    _tracebackListenersCount += newCounted ? 1 : -1;
+                    Interlocked.Add(ref _tracebackListenersCount, newCounted ? 1 : -1);
                 }
 
                 if (EnableTracing != oldEnableTracing) {

tests/suite/modules/system_related/test_sys.py

@@ -233,6 +233,26 @@ class Boom(Exception): pass
         # sys.settrace(None) gave the class body a function's call target.
         self.assertTrue(issubclass(f(), Exception))
 
+    @skipUnlessIronPython()
+    def test_settrace_function_Xdebug(self):
+        """With -X:Debug, a function defined while tracing is on is traced."""
+        import subprocess
+        code = """import sys
+events = []
+def tracer(frame, event, arg):
+    events.append((event, frame.f_code.co_name))
+    return tracer
+sys.settrace(tracer)
+def f():
+    return 1
+f()
+sys.settrace(None)
+print(('call', 'f') in events)
+"""
+        # Before the fix, defining f under -X:Debug gave it its normal (untraced) delegate, even with tracing on.
+        out = subprocess.check_output([sys.executable, '-X:Debug', '-c', code])
+        self.assertEqual(out.strip(), b'True')
+
     @skipUnlessIronPython()
     @unittest.expectedFailure # dummy getrefcount removed in https://github.com/IronLanguages/ironpython3/pull/577
     def test_getrefcount(self):