Multi-thread pulling of out put from AsyncProcess #8549
Conversation
We are seeing hangs on Windows when SwiftPM tests itself. Looking at how AsyncProcess pulls stdout and stderr, I can see that there may be conditions where the streams aren't fully drained causing the hang. The proper way to do this, like it's done on the other platforms is to create a thread for each stream.
dschaefer2
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
xedin,
shawnhyam,
bripeticca,
plemarquand,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
A fatal error was happening in the PackageLoadingTests. Removed the caching of the manifest loader so that if it fails it can properly throw an exception.
|
@swift-ci please test |
There was a problem hiding this comment.
Heh, I've lost count of how many times I've fixed this same problem with uses of Process. Really looking forward to adopting swift-subprocess everywhere, which is async-native.
Looks like readabilityHandler is invoked on a private serial Dispatch queue owned by the FileHandle; I'm not sure this is a scenario where a hang could be caused, but it's hard to tell since the Swift Concurrency / GCD interaction is complex. This approach is worth trying though, since a pthread provides a pretty strong independence guarantee.
|
@swift-ci test windows |
| @@ -17,7 +17,6 @@ import _InternalTestSupport | |||
| import XCTest | |||
|
|
|||
| class PackageDescriptionLoadingTests: XCTestCase, ManifestLoaderDelegate { | |||
| lazy var manifestLoader = ManifestLoader(toolchain: try! UserToolchain.default, delegate: self) | |||
There was a problem hiding this comment.
suggestion: can we instantiate manifestLoader in a setUp(...) call to ensure each test instantiates/creates its own object?
| @@ -17,7 +17,6 @@ import _InternalTestSupport | |||
| import XCTest | |||
|
|
|||
| class PackageDescriptionLoadingTests: XCTestCase, ManifestLoaderDelegate { | |||
| lazy var manifestLoader = ManifestLoader(toolchain: try! UserToolchain.default, delegate: self) | |||
| var parsedManifest = ThreadSafeBox<AbsolutePath>() | |||
There was a problem hiding this comment.
suggestion: can we instantiate parsedManifest in a setUp(...) call to ensure each test instantiates/creates its own object?
|
Still hanging, even removing the --parallel. Trying one more attempt at going pure win32. But, yes, subprocess would come in handy right now. |
To make sure it's clear what AsyncProcess is doing on Windows, Replace the use of Foundation's Process with direct calls to win32's CreateProcessW.
|
Consistently seeing the GitRepository tests hanging. Will try a couple of more times to confirm. |
|
@swift-ci please test self hosted windows |
|
Huh, that was it. I'm going to abandon this and go back to using Foundation.Process but with the multi-threaded reads. And then make sure multiple gits aren't running at the same time. |
We are seeing hangs on Windows when SwiftPM tests itself. Looking at how AsyncProcess pulls stdout and stderr, I can see that there may be conditions where the streams aren't fully drained causing the hang. The proper way to do this, like it's done on the other platforms is to create a thread for each stream.