Basics: allow reusing cancellator handlers from SwiftTool
#6396
Conversation
These signal handlers are useful to other tools that don't have access to `SwiftTool`. For example, `DestinationCommand` would benefit from this as it needs to set up its own cancellator for handling archiver processes.
MaxDesiatov
requested review from
abertelrud,
neonichu and
tomerd
as code owners
|
@swift-ci smoke test |
|
no sure how I feel about this moving to the cancellator. maybe a new static utility? |
|
if we are moving to a utility like this we should also add precondition to make sure it is not called more than once |
I've added a check for this via
Such utility would still need to take a instance of a cancellator as an argument, and then is there much difference between taking it as a function argument, or via |
|
@swift-ci smoke test |
|
@swift-ci test windows |
|
@swift-ci smoke test |
|
@swift-ci test windows |
|
@swift-ci smoke test |
|
@swift-ci test windows |
Sources/Basics/Cancellator.swift
Outdated
| interruptSignalSource.resume() | ||
| #endif | ||
|
|
||
| Self.isSignalHandlerInstalled = true |
There was a problem hiding this comment.
non blocking but this is not thread safe
There was a problem hiding this comment.
I've added a lock for the whole function body to make sure that there's no race condition when installing these handlers.
Sources/CoreCommands/SwiftTool.swift
Outdated
| internal init( | ||
| outputStream: OutputByteStream, | ||
| options: GlobalOptions, | ||
| shouldInstallSignalHandlers: Bool = true, |
There was a problem hiding this comment.
we can consider adding this to global options
There was a problem hiding this comment.
GlobalOptions is dedicated to properties using @Option property wrappers passed from ArgumentParser. Would this ToolWorkspaceConfiguration type values of which are passed as arguments to this initializer also work?
| return try SwiftTool( | ||
| outputStream: outputStream, | ||
| options: options, | ||
| shouldInstallSignalHandlers: false, |
There was a problem hiding this comment.
feels like this belongs into options
There was a problem hiding this comment.
Moved to ToolWorkspaceConfiguration for now per my comment above.
|
@swift-ci smoke test |
|
@swift-ci test windows |
|
@swift-ci test windows |
|
@swift-ci smoke test |
|
@swift-ci test windows |
|
@swift-ci test windows |
|
@swift-ci smoke test |
These signal handlers are useful to other tools that don't have access to `SwiftTool`. For example, `DestinationCommand` would benefit from this as it needs to set up its own cancellator for handling archiver processes. Moved signal handler setup to a new `installSignalHandlers` function on `Cancellator`. Made captures of `Cancellator` in handler closures weak so that it's not referenced indefinitely and is released when no longer needed.
Motivation:
These signal handlers are useful to other tools that don't have access to
SwiftTool. For example,DestinationCommandwould benefit from this as it needs to set up its own cancellator for handling archiver processes.Modifications:
Moved signal handler setup to a new
installSignalHandlersfunction onCancellator. Made captures ofCancellatorin handler closures weak so that it's not referenced indefinitely and is released when no longer needed.Result:
Cancellator/installSignalHandlercan be used in more places.