Allow swift-build to have async entrypoint
#7369
Conversation
Due to a bug in Swift 5.9, we can't have multiple executables with `async` entrypoints linked into test products. This was the reason why we weren't able to make `swift-build` entrypoint `async`. Since we have a single `swift-package-manager` executable anyway, we can link that one into tests instead, and pass "simulated" executable name via an environment variable. This allows us to work around the bug and start using structured concurrency from `swift-build` entrypoint.
|
@swift-ci test |
|
@swift-ci test |
|
@swift-ci test windows |
|
@swift-ci clean test windows |
|
@swift-ci test windows |
…xd/async-swift-build # Conflicts: # Sources/Commands/SwiftBuildCommand.swift # Sources/swift-build/Entrypoint.swift # Sources/swift-package-manager/SwiftPM.swift
|
@swift-ci test |
|
@swift-ci test windows |
| // FIXME: not sure how this was supposed to work previously, since `swift-run` uses `execv` that doesn't inherit | ||
| // signal handlers, and the spawned process doesn't install signal handlers on its own. `async` effect on | ||
| // `@main` arguably makes it behave correctly? |
There was a problem hiding this comment.
It seems this issue is not only a problem in tests but causes a real problem. I could reproduce this uninterruptable swift-run when invoking just built swift-package-manager through symlink named swift-run.
Based on my quick research, it seems like libdispatch blocks SIGINT by sigmask and spawns a dedicated thread to catch those signals. I think that's why we haven't seen this before with sync entrypoints.
So the problematic code can be minimized as follows.
import Dispatch
import Foundation
DispatchQueue.main.async {
let program = "/usr/bin/sleep"
let args = ["sleep", "10"]
let cArgs = UnsafeMutablePointer<UnsafeMutablePointer<Int8>?>.allocate(capacity: args.count + 1)
for (i, arg) in args.enumerated() {
cArgs[i] = strdup(arg)
}
cArgs[args.count] = nil
execv(program, cArgs)
perror("execv")
}
dispatchMain()I confirmed we can unblock signals as follows, but I'm not 100% sure it's a safe way.
var mask = sigset_t()
sigemptyset(&mask)
pthread_sigmask(SIG_SETMASK, &mask, nil)
execv(program, cArgs)There was a problem hiding this comment.
IIUC Dispatch blocks delivery of SIGINT whenever some code is executed in DispatchQueue.main.async, is that correct? Isn't it a bug in Dispatch then and could be fixed on their side?
There was a problem hiding this comment.
No, libdispatch blocks deliverry of SIGINT during the whole program execution, but it creates a dedicated thread to listen to signals before blocking the signals in the main thread. SIGINT is usually handled by the dedicated thread and I guess the actual signal handling works are scheduled on some dispatch queues. So this is not a bug in libdispatch at all, but they don't provide a graceful way to reset signal mask states. Therefore I suggested resetting masks manually just before execv in SwiftPM as one of the options.
There was a problem hiding this comment.
That's a good point, I've addressed it in a subsequent commit. The signals reset code had to be slightly different for it to work in our swift run use case and I'm also closing file descriptors as suggested by @weissi
|
@swift-ci test |
|
@swift-ci clean test windows |
|
|
||
| // 2. close all file descriptors. | ||
| for i in 3..<getdtablesize() { | ||
| close(i) |
There was a problem hiding this comment.
Do we have any open fds managed by SwiftPM at this point? My concern is that this might corrupt the state of underlying libraries like libc and cause other problems for the case when exec fails for some reason. I think closing fds is not a part of this function's responsibilities but should be handled by those fds owners using O_CLOEXEC.
There was a problem hiding this comment.
Do we have any open fds managed by SwiftPM at this point?
I don't think we should, but I don't have any certainty our swift-tools-support-core or swift-corelibs-foundation APIs don't leak anything. I could see the FileHandle API or some reference or escapable type captured somewhere with no explicit close call on it.
There was a problem hiding this comment.
I think the discussion point here is whether to prioritize potential state corruption after the exec fails or potential fd leak during the exec'ed program execution. IMO the former issue is more critical than the latter since we don't have a large number of open fd's here and emitting proper error diagnostics when a problem occurs is essential for further investigation.
There was a problem hiding this comment.
IIUC the argument that Johannes is making is that bash is doing it this way and it works fine for them. Whatever state underlying libraries have, it'll be gone with execv call anyway https://github.com/bminor/bash/blob/f3b6bd19457e260b65d11f2712ec3da56cef463f/execute_cmd.c#L6162C1-L6173
There was a problem hiding this comment.
Yes, that's what UNIX programs are expected to do if they exec: close the file descriptors that they're not intending to pass on. LGTM
There was a problem hiding this comment.
Okay, that makes sense to me 👍
|
@swift-ci test windows |
Due to a bug in Swift 5.9, we can't have multiple executables with `async` entrypoints linked into test products. This was the reason why we weren't able to make `swift-build` entrypoint `async`. Since we have a single `swift-package-manager` executable anyway, we can link that one into tests instead, and pass "simulated" executable name via an environment variable. This allows us to work around the bug and start using structured concurrency from `swift-build` entrypoint. This change is a prerequisite for swiftlang#7367.
Due to a bug in Swift 5.9, we can't have multiple executables with `async` entrypoints linked into test products. This was the reason why we weren't able to make `swift-build` entrypoint `async`. Since we have a single `swift-package-manager` executable anyway, we can link that one into tests instead, and pass "simulated" executable name via an environment variable. This allows us to work around the bug and start using structured concurrency from `swift-build` entrypoint. This change is a prerequisite for swiftlang#7367.
Due to a bug in Swift 5.9, we can't have multiple executables with
asyncentrypoints linked into test products. This was the reason why we weren't able to makeswift-buildentrypointasync. Since we have a singleswift-package-managerexecutable anyway, we can link that one into tests instead, and pass "simulated" executable name via an environment variable. This allows us to work around the bug and start using structured concurrency fromswift-buildentrypoint.Also converted
SwiftRunCommandentrypoint toasyncto reliably reproduce possible regressions highlighted byRunCommandTests.testSwiftRunSIGINT. These regressions were handled by resetting signals masks and closing file descriptors before a subsequent call toexecv.This change is a prerequisite for #7367.