Skip to content

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Oct 20, 2021

motivation: DispatchSignalSource is a supported cross platform signal handing API, we do not need a custom one in TSC

changes:

  • replace use TSC's InterruptHandler with DispatchSignalSource
  • fix an issue where some git processes did not get registered with the ProcessSet for termination candidates

@tomerd
Copy link
Contributor Author

tomerd commented Oct 20, 2021

@tomerd
Copy link
Contributor Author

tomerd commented Oct 20, 2021

@swift-ci please smoke test

motivation: DispatchSignalSource is a supported cross platform signal handing API, we do not need a custom one in TSC

changes:
* replace use TSC's InterruptHandler with DispatchSignalSource
* fix an issue where some git processes did not get registered with the ProcessSet for termination candidates
@tomerd tomerd force-pushed the refactor/signal-handler branch from e899ccc to 0bd58aa Compare October 20, 2021 23:20
@tomerd tomerd force-pushed the refactor/signal-handler branch from 0bd58aa to 72816d1 Compare October 20, 2021 23:27
@tomerd
Copy link
Contributor Author

tomerd commented Oct 20, 2021

@swift-ci please smoke test

@tomerd tomerd self-assigned this Oct 20, 2021
@tomerd
Copy link
Contributor Author

tomerd commented Oct 20, 2021

cc @compnerd for impact on Windows

@tomerd tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Oct 20, 2021
@compnerd
Copy link
Member

I think that this might work:

diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift
index 184d117d..17baf7e6 100644
--- a/Sources/Commands/SwiftTool.swift
+++ b/Sources/Commands/SwiftTool.swift
@@ -26,6 +26,10 @@ import TSCUtility
 import Workspace
 import XCBuildSupport
 
+#if os(Windows)
+import WinSDK
+#endif
+
 typealias Diagnostic = TSCBasic.Diagnostic
 
 private class ToolWorkspaceDelegate: WorkspaceDelegate {
@@ -297,6 +301,10 @@ public class SwiftTool {
     /// The current build system reference. The actual reference is present only during an active build.
     let buildSystemRef: BuildSystemRef
 
+#if os(Windows)
+    static var storage: (ProcessSet, BuildSystemRef)?
+#endif
+
     /// The execution status of the tool.
     var executionStatus: ExecutionStatus = .success
 
@@ -341,6 +349,22 @@ public class SwiftTool {
             let buildSystemRef = BuildSystemRef()
 
             // trap SIGINT to terminate sub-processes, etc
+#if os(Windows)
+            _ = SetConsoleCtrlHandler({ _ in
+                // Terminate all processes on receiving an interrupt signal.
+                SwiftTool.storage?.0.terminate()
+                SwiftTool.storage?.1.buildSystem?.cancel()
+
+                // Reset the handler.
+                _ = SetConsoleCtrlHandler(nil, false)
+
+                // Exit as if by signal()
+                TerminateProcess(GetCurrentProcess(), 3)
+
+                return true
+            }, true)
+            SwiftTool.storage = (processSet, buildSystemRef)
+#else
             signal(SIGINT, SIG_IGN)
             let interruptSignalSource = DispatchSource.makeSignalSource(signal: SIGINT)
             interruptSignalSource.setEventHandler {
@@ -351,10 +375,7 @@ public class SwiftTool {
                 processSet.terminate()
                 buildSystemRef.buildSystem?.cancel()
 
-#if os(Windows)
-                // Exit as if by signal()
-                TerminateProcess(GetCurrentProcess(), 3)
-#elseif os(macOS) || os(OpenBSD)
+#if os(macOS) || os(OpenBSD)
                 // Install the default signal handler.
                 var action = sigaction()
                 action.__sigaction_u.__sa_handler = SIG_DFL
@@ -376,6 +397,7 @@ public class SwiftTool {
 #endif
             }
             interruptSignalSource.resume()
+#endif
 
             self.processSet = processSet
             self.buildSystemRef = buildSystemRef

@tomerd
Copy link
Contributor Author

tomerd commented Oct 22, 2021

@compnerd can you verify the windows patch so we can merge?

@tomerd
Copy link
Contributor Author

tomerd commented Oct 22, 2021

@swift-ci please smoke test

@compnerd
Copy link
Member

Need to fix t-s-c first, your changes broke windows

@compnerd
Copy link
Member

@compnerd
Copy link
Member

I built and ran with https://github.com/compnerd/swift-package-manager/pull/new/refactor/signal-handler ... that seems to work for me. Would you mind just cherry-picking the top commit from that. Once we have swiftlang/swift-tools-support-core#250, I think that this should be okay for Windows.

@tomerd tomerd merged commit 0555e52 into swiftlang:main Oct 23, 2021
abertelrud pushed a commit to abertelrud/swift-package-manager that referenced this pull request Oct 25, 2021
motivation: DispatchSignalSource is a supported cross platform signal handing API, we do not need a custom one in TSC

changes:
* replace use TSC's InterruptHandler with DispatchSignalSource
* fix an issue where some git processes did not get registered with the ProcessSet for termination candidates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants