[Concurrency] Further test cleanups, making sure modes interop properly

Author: ktoso

stdlib/public/Concurrency/Actor.cpp

@@ -337,33 +337,47 @@ static IsCurrentExecutorCheckMode isCurrentExecutorMode =
 
 
 // Shimming call to Swift runtime because Swift Embedded does not have
-// these symbols defined
+// these symbols defined.
 bool swift_bincompat_useLegacyNonCrashingExecutorChecks() {
 #if !SWIFT_CONCURRENCY_EMBEDDED
   return swift::runtime::bincompat::
       swift_bincompat_useLegacyNonCrashingExecutorChecks();
-#endif
+#else
   return false;
+#endif
 }
 
 // Check override of executor checking mode.
 static void checkIsCurrentExecutorMode(void *context) {
   auto useLegacyMode =
       swift_bincompat_useLegacyNonCrashingExecutorChecks();
 
+  fprintf(stderr, "[%s:%d](%s) useLegacyMode FROM BINCOMPAT = %s\n", __FILE_NAME__, __LINE__, __FUNCTION__,
+          useLegacyMode ? "Legacy_NoCheckIsolated_NonCrashing" : "Default_UseCheckIsolated_AllowCrash");
+
   // Potentially, override the platform detected mode, primarily used in tests.
 #if SWIFT_STDLIB_HAS_ENVIRON
-  if (const char *modeStr =
-          runtime::environment::concurrencyIsCurrentExecutorLegacyModeOverride()) {
-    if (modeStr) {
-      if (strcmp(modeStr, "nocrash") == 0) {
-        useLegacyMode = true;
-      } else if (strcmp(modeStr, "crash") == 0) {
-        useLegacyMode = false;
-      } // else, just use the platform detected mode
-    }
+  // if (const char *modeStr = runtime::environment::concurrencyIsCurrentExecutorLegacyModeOverride()) {
+  if (const char *modeStr = getenv("SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE")) {
+    if (strcmp(modeStr, "nocrash") == 0 || strcmp(modeStr, "legacy") == 0) {
+      fprintf(stderr, "[%s:%d](%s) OVERRIDE TO USE [LEGACY] MODE\n", __FILE_NAME__, __LINE__, __FUNCTION__);
+      useLegacyMode = true;
+    } else if (strcmp(modeStr, "crash") == 0 || strcmp(modeStr, "swift6") == 0) {
+      fprintf(stderr, "[%s:%d](%s) OVERRIDE TO USE [CRASH] MODE\n", __FILE_NAME__, __LINE__, __FUNCTION__);
+      useLegacyMode = false;
+    } // else, just use the platform detected mode
+
+    fprintf(stderr, "[%s:%d](%s) useLegacyMode override from ENV = %s (mode string = %s)\n",
+        __FILE_NAME__, __LINE__, __FUNCTION__,
+        useLegacyMode ? "Legacy_NoCheckIsolated_NonCrashing"
+                      : "Default_UseCheckIsolated_AllowCrash",
+        modeStr);
+  } else {
+    fprintf(stderr, "[%s:%d](%s) no override of useLegacyMode...\n",
+            __FILE_NAME__, __LINE__, __FUNCTION__);
   }
 #endif // SWIFT_STDLIB_HAS_ENVIRON
+
   isCurrentExecutorMode = useLegacyMode ? Legacy_NoCheckIsolated_NonCrashing
                                         : Default_UseCheckIsolated_AllowCrash;
 }
@@ -383,14 +397,27 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
   static swift::once_t checkModeToken;
   swift::once(checkModeToken, checkIsCurrentExecutorMode, nullptr);
 
+  auto modeString = isCurrentExecutorMode == Legacy_NoCheckIsolated_NonCrashing
+                        ? "Legacy_NoCheckIsolated_NonCrashing"
+                        : "Default_UseCheckIsolated_AllowCrash";
+  fprintf(stderr, "[%s:%d](%s) MODE = isCurrentExecutorMode = %s\n", __FILE_NAME__, __LINE__, __FUNCTION__,
+          modeString);
+
+  fprintf(stderr, "[%s:%d](%s) expected executor is main: %d\n", __FILE_NAME__, __LINE__, __FUNCTION__,
+          expectedExecutor.isMainExecutor());
+  fprintf(stderr, "[%s:%d](%s) thread is main: %d\n", __FILE_NAME__, __LINE__, __FUNCTION__,
+          isExecutingOnMainThread());
+
   if (!current) {
+    fprintf(stderr, "[%s:%d](%s) no current\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     // We have no current executor, i.e. we are running "outside" of Swift
     // Concurrency. We could still be running on a thread/queue owned by
     // the expected executor however, so we need to try a bit harder before
     // we fail.
 
     // Are we expecting the main executor and are using the main thread?
     if (expectedExecutor.isMainExecutor() && isExecutingOnMainThread()) {
+      fprintf(stderr, "[%s:%d](%s) main executor; main thread >>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
       // Due to compatibility with pre-checkIsolated code, we cannot remove
       // this special handling. CheckIsolated can handle this if the expected
       // executor is the main queue / main executor, however, if we cannot call
@@ -399,15 +426,23 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
       return true;
     }
 
+    fprintf(stderr, "[%s:%d](%s) have current executor = %p\n", __FILE_NAME__, __LINE__, __FUNCTION__,
+            current);
+
     // Otherwise, as last resort, let the expected executor check using
     // external means, as it may "know" this thread is managed by it etc.
     if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+      fprintf(stderr, "[%s:%d](%s) allow crash mode, checkIsolated call \n", __FILE_NAME__, __LINE__, __FUNCTION__);
       swift_task_checkIsolated(expectedExecutor);
       // checkIsolated did not crash, so we are on the right executor, after all!
+      fprintf(stderr, "[%s:%d](%s) checkIsolated call passed >>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
       return true;
+    } else {
+      fprintf(stderr, "[%s:%d](%s) cannot invoke 'checkIsolated'\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     }
 
     assert(isCurrentExecutorMode == Legacy_NoCheckIsolated_NonCrashing);
+    fprintf(stderr, "[%s:%d](%s) >>>>> false\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     return false;
   }
 
@@ -417,39 +452,43 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
   // We assume executors do not come-and-go appearing under the same address,
   // and treat pointer equality of executors as good enough to assume the executor.
   if (currentExecutor == expectedExecutor) {
+    fprintf(stderr, "[%s:%d](%s) executor pointer equal; >>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     return true;
   }
 
   // Fast-path, specialize the common case of comparing two main executors.
   if (currentExecutor.isMainExecutor() && expectedExecutor.isMainExecutor()) {
+    fprintf(stderr, "[%s:%d](%s) both are main executor >>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     return true;
   }
 
-  // If the expected executor is "default" then we should have matched
-  // by pointer equality already with the current executor.
-  if (expectedExecutor.isDefaultActor()) {
-    // If the expected executor is a default actor, it makes no sense to try
-    // the 'checkIsolated' call, it must be equal to the other actor, or it is
-    // not the same isolation domain.
-    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
-      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption");
-    }
-    return false;
-  }
-
-  if (expectedExecutor.isMainExecutor() && !currentExecutor.isMainExecutor()) {
-    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
-      // TODO: Invoke checkIsolated() on "main" SerialQueue once it implements `checkIsolated`, for potentially better/more consistent error messages
-      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption; Expected MainActor executor");
-    }
-    return false;
-  } else if (!expectedExecutor.isMainExecutor() && currentExecutor.isMainExecutor()) {
-    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
-      // TODO: Invoke checkIsolated() on "main" SerialQueue once it implements `checkIsolated`, for potentially better/more consistent error messages
-      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption; Expected not-MainActor executor");
-    }
-    return false;
-  }
+  // FIXME: this was incorrect, if expected is default we need to invoke the checkIsolated
+//  // If the expected executor is "default" then we should have matched
+//  // by pointer equality already with the current executor.
+//  if (expectedExecutor.isDefaultActor()) {
+//    // If the expected executor is a default actor, it makes no sense to try
+//    // the 'checkIsolated' call, it must be equal to the other actor, or it is
+//    // not the same isolation domain.
+//    fprintf(stderr, "[%s:%d](%s) both are main executor >>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
+//    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+//      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption");
+//    }
+//    return false;
+//  }
+
+  // FIXME: all of this is wrong, we need to keep trying complex equality;
+  // as something may not be EQUAL to the main executor but still be equivalent to it.
+//  if (expectedExecutor.isMainExecutor() && !currentExecutor.isMainExecutor()) {
+//    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+//      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption; Expected MainActor executor");
+//    }
+//    return false;
+//  } else if (!expectedExecutor.isMainExecutor() && currentExecutor.isMainExecutor()) {
+//    if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+//      swift_Concurrency_fatalError(0, "Incorrect actor executor assumption; Expected not-MainActor executor");
+//    }
+//    return false;
+//  }
 
   if (expectedExecutor.isComplexEquality()) {
     if (currentExecutor.getIdentity() && expectedExecutor.getIdentity() &&
@@ -469,6 +508,7 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
       // it and return; if it was false, we need to give checkIsolated another
       // chance to check.
       if (isSameExclusiveExecutionContextResult) {
+        fprintf(stderr, "[%s:%d](%s) is same exclusive execution context! >>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
         return true;
       }
     }
@@ -496,12 +536,15 @@ static bool swift_task_isCurrentExecutorImpl(SerialExecutorRef expectedExecutor)
   // presence of a Task.
   // compat_invoke_swift_task_checkIsolated(expectedExecutor);
   if (isCurrentExecutorMode == Default_UseCheckIsolated_AllowCrash) {
+    fprintf(stderr, "[%s:%d](%s) last chance; invoke 'checkIsolated'\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     swift_task_checkIsolated(expectedExecutor);
+
     // The checkIsolated call did not crash, so we are on the right executor.
+    fprintf(stderr, "[%s:%d](%s) checkIsolated did not crash; >>>>>> true\n", __FILE_NAME__, __LINE__, __FUNCTION__);
     return true;
   }
 
-  assert(isCurrentExecutorMode == Legacy_NoCheckIsolated_NonCrashing);
+  fprintf(stderr, "[%s:%d](%s) cannot invoke 'checkIsolated' assume not equal >>>>>> false \n", __FILE_NAME__, __LINE__, __FUNCTION__);
   return false;
 }
 
@@ -524,9 +567,30 @@ static void checkUnexpectedExecutorLogLevel(void *context) {
   if (!levelStr)
     return;
 
+  auto isSameExecutorLegacyMode =
+      swift_bincompat_useLegacyNonCrashingExecutorChecks();
+
   long level = strtol(levelStr, nullptr, 0);
-  if (level >= 0 && level < 3)
-    unexpectedExecutorLogLevel = level;
+  if (level >= 0 && level < 3) {
+    if (isSameExecutorLegacyMode) {
+      // legacy mode permits doing nothing or just logging, since the method
+      // used to perform the check itself is not going to crash:
+      unexpectedExecutorLogLevel = level;
+    } else {
+      // We are in swift6/crash mode of isSameExecutor... which means that
+      // rather than returning false, that method will always CRASH when an
+      // executor mismatch is discovered. This means that it is not possible
+      // to support logging or warning 'unexpected executor log level' in this mode.
+      if (level > 1) {
+        fprintf(stderr, "[%s:%d](%s) [unexpected executor] ONLY CRASH MODE IS OK\n", __FILE_NAME__, __LINE__, __FUNCTION__);
+        unexpectedExecutorLogLevel = level;
+      } else {
+        fprintf(stderr,
+                "[%s:%d](%s) [unexpected executor] HAD TO IGNORE LEVEL [%s] CRASH MODE IS OK\n",
+                level, __FILE_NAME__, __LINE__, __FUNCTION__);
+      }
+    }
+  }
 #endif // SWIFT_STDLIB_HAS_ENVIRON
 }
 

stdlib/public/runtime/Bincompat.cpp

@@ -255,6 +255,19 @@ bool useLegacySwiftObjCHashing() {
 #endif
 }
 
+// Controls legacy mode for the 'swift_task_isCurrentExecutorImpl' runtime function.
+//
+// In "legacy" / "no crash" mode:
+// * The `swift_task_isCurrentExecutorImpl` cannot crash
+// * This means cases where no "current" executor is present cannot be diagnosed correctly
+//    * The runtime can NOT use 'SerialExecutor/checkIsolated'
+//    * The runtime can NOT use 'dispatch_precondition' which is able ot handle some dispatch and main actor edge cases
+//
+// New behavior in "swift6" "crash" mode:
+// * The 'swift_task_isCurrentExecutorImpl' will CRASH rather than return 'false'
+// * This allows the method to invoke 'SerialExecutor/checkIsolated'
+//   * Which is allowed to call 'dispatch_precondition' and handle "on dispatch queue but not on Swift executor" cases
+//
 // FIXME(concurrency): Once the release is announced, adjust the logic detecting the SDKs
 bool swift_bincompat_useLegacyNonCrashingExecutorChecks() {
 #if BINARY_COMPATIBILITY_APPLE

test/Concurrency/Runtime/actor_assert_precondition_executor_checkIsolated_bincompat.swift renamed to test/Concurrency/Runtime/actor_assert_precondition_executor_checkIsolated_bincompat_crash_swift_6_mode.swift

@@ -1,4 +1,7 @@
-// RUN: %target-run-simple-swift(-parse-as-library -Xfrontend -disable-availability-checking) | %FileCheck %s
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library %s -o %t/a.out
+// RUN: %target-codesign %t/a.out
+// RUN: %env-SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=swift6 %target-run %t/a.out
 
 // REQUIRES: executable_test
 // REQUIRES: concurrency
@@ -11,6 +14,8 @@
 // UNSUPPORTED: use_os_stdlib
 // UNSUPPORTED: freestanding
 
+import StdlibUnittest
+
 final class NaiveQueueExecutor: SerialExecutor {
   init() {}
 
@@ -38,32 +43,26 @@ actor ActorOnNaiveQueueExecutor {
     self.executor.asUnownedSerialExecutor()
   }
 
+  // Executes on global pool, but our `checkIsolated` impl pretends
+  // that it is the same executor by never crashing.
   nonisolated func checkPreconditionIsolated() async {
     print("Before preconditionIsolated")
     self.preconditionIsolated()
     print("After preconditionIsolated")
-
-    print("Before assumeIsolated")
-    self.assumeIsolated { iso in
-      print("Inside assumeIsolated")
-    }
-    print("After assumeIsolated")
   }
 }
 
 @main struct Main {
   static func main() async {
-    if #available(SwiftStdlib 6.0, *) {
-      let actor = ActorOnNaiveQueueExecutor()
-      await actor.checkPreconditionIsolated()
-      // CHECK: Before preconditionIsolated
-      // CHECK-NEXT: checkIsolated: pretend it is ok!
-      // CHECK-NEXT: After preconditionIsolated
+    let tests = TestSuite("AssertPreconditionIsolationTests")
 
-      // CHECK-NEXT: Before assumeIsolated
-      // CHECK-NEXT: checkIsolated: pretend it is ok!
-      // CHECK-NEXT: Inside assumeIsolated
-      // CHECK-NEXT: After assumeIsolated
+    if #available(SwiftStdlib 6.0, *) {
+      tests.test("[swift6+checkIsolated] Isolation assured by invoking 'checkIsolated'") {
+        let actor = ActorOnNaiveQueueExecutor()
+        await actor.checkPreconditionIsolated()
+      }
     }
+    
+    await runAllTestsAsync()
   }
 }

test/Concurrency/Runtime/actor_assert_precondition_executor_checkIsolated_bincompat_nocrash_legacy_mode.swift

@@ -0,0 +1,70 @@
+// RUN: %empty-directory(%t)
+// RUN: %target-build-swift -Xfrontend -disable-availability-checking %import-libdispatch -parse-as-library %s -o %t/a.out
+// RUN: %target-codesign %t/a.out
+// RUN: %env-SWIFT_IS_CURRENT_EXECUTOR_LEGACY_MODE_OVERRIDE=legacy %target-run %t/a.out
+
+// REQUIRES: executable_test
+// REQUIRES: concurrency
+// REQUIRES: concurrency_runtime
+
+// REQUIRES: libdispatch
+
+// UNSUPPORTED: back_deployment_runtime
+// UNSUPPORTED: back_deploy_concurrency
+// UNSUPPORTED: use_os_stdlib
+// UNSUPPORTED: freestanding
+
+import StdlibUnittest
+
+final class NaiveQueueExecutor: SerialExecutor {
+  init() {}
+
+  public func enqueue(_ job: consuming ExecutorJob) {
+    job.runSynchronously(on: self.asUnownedSerialExecutor())
+  }
+
+  public func asUnownedSerialExecutor() -> UnownedSerialExecutor {
+    UnownedSerialExecutor(ordinary: self)
+  }
+
+  func checkIsolated() {
+    print("checkIsolated: pretend it is ok!")
+  }
+}
+
+actor ActorOnNaiveQueueExecutor {
+  let executor: NaiveQueueExecutor
+
+  init() {
+    self.executor = NaiveQueueExecutor()
+  }
+
+  nonisolated var unownedExecutor: UnownedSerialExecutor {
+    self.executor.asUnownedSerialExecutor()
+  }
+
+  // Executes on global pool, but our `checkIsolated` impl pretends
+  // that it is the same executor by never crashing.
+  nonisolated func checkPreconditionIsolated() async {
+    print("Before preconditionIsolated")
+    self.preconditionIsolated()
+    print("After preconditionIsolated")
+  }
+}
+
+@main struct Main {
+  static func main() async {
+    let tests = TestSuite("AssertPreconditionIsolationTests")
+
+    if #available(SwiftStdlib 6.0, *) {
+      tests.test("[legacy mode] expect crash since unable to invoke 'checkIsolated'") {
+        expectCrashLater() // In legacy mode we do NOT invoke 'checkIsolated' and therefore will crash
+
+        let actor = ActorOnNaiveQueueExecutor()
+        await actor.checkPreconditionIsolated()
+      }
+    }
+
+    await runAllTestsAsync()
+  }
+}