Fix SIGINT handling during `swift run` #4224

kkebo · 2022-03-17T13:12:57Z

I fixed SIGINT handling during swift run.

Motivation:

Since Swift 5.6, Ctrl-C no longer works during swift run.

The issue was introduced in #3815. That PR added signal(SIGINT, SIG_IGN) for handling SIGINT by DispatchSourceSignal. That's no problem, but exec(...) replaces the process, so once exec(...) is called, DispatchSourceSignal can't handle SIGINT anymore and no one can handle it. Therefore, it is necessary to cancel signal(SIGINT, SIG_IGN) before calling exec(...).

Modifications:

I created a wrapper function of exec(...) that calls signal(SIGINT, SIG_DFL) just before exec(...) in order to cancel signal(SIGINT, SIG_IGN), and use it from SwiftRunTool (2 methods), WatchmanHelper, and SnippetCard.

Result:

Ctrl-C will work again during swift run.

tomerd · 2022-03-17T16:18:08Z

Sources/Commands/SwiftRunTool.swift

@@ -158,6 +166,9 @@ public struct SwiftRunTool: SwiftCommand {
                    try ProcessEnv.chdir(swiftTool.originalWorkingDirectory)
                }

+                // SwiftTool can't handle signals anymore, so reset the signal handler to SIG_DFL before call exec()
+                signal(SIGINT, SIG_DFL)


thanks for the fix @kkk669 makes sense. could we maybe create a wrapper for exec in this file that reset the signal handler first then delegates to the "real" exec? this way we can keep it DRYer. wdyt?

I agree with you. Should I create a wrapper just for SwiftRunTool? Or should I create it so that it can be used from other commands? If there is a possibility of using exec for other commands in the future, I feel that a wrapper should be created for the entire Commands target.

good point. lets add to SwiftTool and then we can call that from any other tool

Thank you for the direction. I will try.

I created a wrapper function in ~~bff14cc~~ 72d8c92.

With this change, not only SwiftRunTool but also WatchmanHelper and SnippetCard now call the wrapper.

I wasn't sure whether to create SwiftTool.exec or global exec, but in the end I went with the latter. Because I believe that it will prevent mistakes like using TSCBasic.exec in the future.

tomerd · 2022-03-17T16:19:49Z

Sources/Commands/SwiftRunTool.swift

+import Darwin
+#else
+import Glibc
+#endif


#if canImport(WinSDK) import WinSDK #elseif canImport(Darwin) import Darwin #elseif canImport(Glibc) import Glibc #endif

more idiomatic / correct?

Yes, I also think so. I'll change it to that form.

SwiftTool.swift has the same problem.

https://github.com/apple/swift-package-manager/blob/0e804edf19e05ddd02d05c1534d4ceea01117e5f/Sources/Commands/SwiftTool.swift#L26_L32

These imports were removed in ~~bff14cc~~ 72d8c92.

tomerd · 2022-03-17T17:49:35Z

@swift-ci smoke test

tomerd · 2022-03-17T17:57:43Z

Sources/Commands/SwiftTool.swift

@@ -286,6 +286,14 @@ extension SwiftCommand {
    public static var _errorLabel: String { "error" }
 }

+/// A safe wrapper of TSCBasic.exec.
+static func exec(path: String, args: [String]) throws -> Never {


remove static, or make into a static func of SwiftTool

Oh, sorry. I fixed it.

https://github.com/apple/swift-package-manager/compare/bff14cc897f5fe974013a6fac0a86850cc200fc8..72d8c9225c9d0822a61213110ae33de5c68f0e54

In the Commands module, this wrapper function is called by default if you write `try exec(path: ..., args: ...)`.

tomerd · 2022-03-17T18:06:52Z

@swift-ci smoke test

tomerd · 2022-03-17T23:13:44Z

thanks again @kkk669 could you also crate a cherry-pick PR against the release/5.6 branch, this may be worth including in a patch version

* Fix SIGINT handling during `swift run` * Create a safe wrapper of TSCBasic.exec * In the Commands module, this wrapper function is called by default if you write `try exec(path: ..., args: ...)`. Co-authored-by: Kenta Kubo <kkk669@users.noreply.github.com>

kkebo · 2022-03-18T00:40:40Z

@tomerd Thank you for merging this. I've created #4231.

compnerd · 2022-03-18T14:38:47Z

This broke the windows build: https://ci-external.swift.org/job/oss-swift-windows-toolchain-x86_64-vs2019/1178/console

kkebo · 2022-03-18T14:55:44Z

@compnerd Thank you for mentioning it. I'll investigate it. Currently, I'm preparing a Windows VM.

kkebo · 2022-03-18T15:13:01Z

Perhaps I found the cause. On Windows, signal(SIGINT, SIG_IGN) isn't called, so I had to enclose the wrapper of TSCBasic.exec with #if !(os(Windows)).

…4235, #4250) (#4231) * Fix SIGINT handling during `swift run` (#4224) * Fix SIGINT handling during `swift run` * Create a safe wrapper of TSCBasic.exec * add test fixture for handling SIGINT on "swift run" (#4250) Co-authored-by: tomer doron <tomer@apple.com> rdar://90574791

Fix SIGINT handling during swift run

3004b7b

kkebo requested review from abertelrud, neonichu, tomerd and elsh as code owners March 17, 2022 13:12

tomerd reviewed Mar 17, 2022

View reviewed changes

tomerd approved these changes Mar 17, 2022

View reviewed changes

tomerd reviewed Mar 17, 2022

View reviewed changes

Create a safe wrapper of TSCBasic.exec

72d8c92

In the Commands module, this wrapper function is called by default if you write `try exec(path: ..., args: ...)`.

kkebo force-pushed the fix-swift-run-sigint branch from bff14cc to 72d8c92 Compare March 17, 2022 18:00

tomerd merged commit 9ad75c5 into apple:main Mar 17, 2022

kkebo deleted the fix-swift-run-sigint branch March 18, 2022 00:25

kkebo mentioned this pull request Mar 18, 2022

[5.6] Fix SIGINT handling during swift run (Cherry-pick of #4224, #4235, #4250) #4231

Merged

kkebo added a commit to kkebo/swift-package-manager that referenced this pull request Mar 18, 2022

Fix Windows build of apple#4224

26967ce

kkebo mentioned this pull request Mar 18, 2022

Fix a build failure on Windows (#4224) #4235

Merged

kkebo added a commit to kkebo/swift-package-manager that referenced this pull request Mar 18, 2022

Fix Windows build of apple#4224

e857589

compnerd pushed a commit that referenced this pull request Mar 19, 2022

Fix Windows build of #4224

e29c65d

kkebo added a commit to kkebo/swift-package-manager that referenced this pull request Mar 19, 2022

Fix Windows build of apple#4224

0c1cc6c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SIGINT handling during `swift run` #4224

Fix SIGINT handling during `swift run` #4224

kkebo commented Mar 17, 2022 •

edited

tomerd Mar 17, 2022

kkebo Mar 17, 2022

tomerd Mar 17, 2022

kkebo Mar 17, 2022

kkebo Mar 17, 2022 •

edited

tomerd Mar 17, 2022

kkebo Mar 17, 2022

tomerd Mar 17, 2022

kkebo Mar 17, 2022 •

edited

tomerd commented Mar 17, 2022

tomerd Mar 17, 2022 •

edited

kkebo Mar 17, 2022

tomerd commented Mar 17, 2022

tomerd commented Mar 17, 2022

kkebo commented Mar 18, 2022

compnerd commented Mar 18, 2022

kkebo commented Mar 18, 2022

kkebo commented Mar 18, 2022

Fix SIGINT handling during swift run #4224

Fix SIGINT handling during swift run #4224

Conversation

kkebo commented Mar 17, 2022 • edited

Motivation:

Modifications:

Result:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkebo Mar 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkebo Mar 17, 2022 • edited

Choose a reason for hiding this comment

tomerd commented Mar 17, 2022

tomerd Mar 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomerd commented Mar 17, 2022

tomerd commented Mar 17, 2022

kkebo commented Mar 18, 2022

compnerd commented Mar 18, 2022

kkebo commented Mar 18, 2022

kkebo commented Mar 18, 2022

Fix SIGINT handling during `swift run` #4224

Fix SIGINT handling during `swift run` #4224

kkebo commented Mar 17, 2022 •

edited

kkebo Mar 17, 2022 •

edited

kkebo Mar 17, 2022 •

edited

tomerd Mar 17, 2022 •

edited