Skip to content

improve process state management #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 25, 2021
Merged

improve process state management #209

merged 2 commits into from
Jun 25, 2021

Conversation

tomerd
Copy link
Contributor

@tomerd tomerd commented Apr 14, 2021

motivation: reports on hang process (mostly tests)

changes:

  • refactor Process to use a state machine to track the process execution state
  • replace use of DispatchQueue with Locks to protect state

rdar://76087764

@tomerd
Copy link
Contributor Author

tomerd commented Apr 14, 2021

brings back #203

@tomerd
Copy link
Contributor Author

tomerd commented Apr 14, 2021 •
edited
Loading

@compnerd I tried to code up the windows part but have no way to test, could you ptal and help get over the finish line

@tomerd tomerd force-pushed the radar/76087764 branch 3 times, most recently from 534a4e0 to ce44b82 Compare April 14, 2021 20:24
@tomerd
Copy link
Contributor Author

tomerd commented Apr 14, 2021

@swift-ci please test

abertelrud
abertelrud reviewed Apr 15, 2021
View reviewed changes
tomerd
tomerd commented Apr 15, 2021
View reviewed changes
@tomerd
Copy link
Contributor Author

tomerd commented Apr 15, 2021

@swift-ci please test

@compnerd
Copy link
Member

This definitely doesn't seem to build on Windows:

S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:419:33: error: cannot use optional chaining on non-optional value of type 'Process'
                            self?.state = .outputReady(stdout: .success(contents), stderr: .success(stderr))
                            ~~~~^

S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:419:73: error: cannot convert value of type 'Data' to expected argument type '[UInt8]'
                            self?.state = .outputReady(stdout: .success(contents), stderr: .success(stderr))
                                                                        ^
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:422:35: error: cannot assign value of type 'Data' to type '[UInt8]'
                        pending = contents
                                  ^~~~~~~~
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:432:33: error: cannot use optional chaining on non-optional value of type 'Process'
                            self?.state = .outputReady(stdout: .success(stdout), stderr: .success(contents))
                            ~~~~^

S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:432:99: error: cannot convert value of type 'Data' to expected argument type '[UInt8]'
                            self?.state = .outputReady(stdout: .success(stdout), stderr: .success(contents))
                                                                                                  ^
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:435:35: error: cannot assign value of type 'Data' to type '[UInt8]'
                        pending = contents
                                  ^~~~~~~~
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:660:16: error: value of type 'UnsafeMutablePointer<FILE>' (aka 'UnsafeMutablePointer<_iobuf>') has no member 'thread'
        stdout.thread?.join()
        ~~~~~~ ^~~~~~
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:661:16: error: value of type 'UnsafeMutablePointer<FILE>' (aka 'UnsafeMutablePointer<_iobuf>') has no member 'thread'
        stderr.thread?.join()
        ~~~~~~ ^~~~~~
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:667:28: error: value of type 'UnsafeMutablePointer<FILE>' (aka 'UnsafeMutablePointer<_iobuf>') has no member 'result'
            output: stdout.result,
                    ~~~~~~ ^~~~~~
S:\SourceCache\swift-tools-support-core\Sources\TSCBasic\Process.swift:668:34: error: value of type 'UnsafeMutablePointer<FILE>' (aka 'UnsafeMutablePointer<_iobuf>') has no member 'result'
            stderrOutput: stderr.result
                          ~~~~~~ ^~~~~~

compnerd
compnerd reviewed Apr 17, 2021
View reviewed changes
Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The waitUntilExit is also something that will need to be updated for your structural changes.

@abertelrud abertelrud self-requested a review April 19, 2021 17:24
@tomerd tomerd self-assigned this May 10, 2021
compnerd added a commit to compnerd/swift-tools-support-core that referenced this pull request May 26, 2021
@compnerd
TSCBasic: unify subprocess handling
16326d6 
This unifies the platforms to Foundation for the process handling.  This
simplifies the logic and gives a single codepath which can possibly
serve as an alternate path to swiftlang#209.
@compnerd compnerd mentioned this pull request May 26, 2021
Closed
@tomerd
Copy link
Contributor Author

tomerd commented May 26, 2021

@swift-ci please test

@compnerd
Copy link
Member

This builds on Windows, but SPM no longer runs.

compnerd added a commit to compnerd/swift-tools-support-core that referenced this pull request May 27, 2021
@compnerd
TSCBasic: unify subprocess handling
051de9b 
This unifies the platforms to Foundation for the process handling.  This
simplifies the logic and gives a single codepath which can possibly
serve as an alternate path to swiftlang#209.
@tomerd
improve process state management
a7a2d86 
motivation: reports on hang process (mostly tests)

changes:
* refactor Process to use a state machine to track the process execution state
* replace use of DispatchQueue with Locks to protect state

rdar://76087764
@tomerd tomerd force-pushed the radar/76087764 branch 2 times, most recently from 6c61c24 to e7fbf88 Compare May 28, 2021 00:39
@tomerd @compnerd
windows fixes
385cd41 
Co-authored-by: Saleem Abdulrasool <[email protected]>
@tomerd
Copy link
Contributor Author

tomerd commented Jun 2, 2021 •
edited
Loading

@compnerd given our work on this, is this ready now in your opinion?

@tomerd
Copy link
Contributor Author

tomerd commented Jun 2, 2021

@swift-ci please test

@tomerd
Copy link
Contributor Author

tomerd commented Jun 9, 2021

@swift-ci please test linux

@tomerd
Copy link
Contributor Author

tomerd commented Jun 9, 2021

@compnerd can you verify this is good to go?

@compnerd
Copy link
Member

@tomerd yeah, given our work on this, I think that this is okay to merge. I've been running with it locally for ~1 week, and I think that the only "downside" to it is that it is a slight bit slower on the startup.

@tomerd tomerd merged commit 4c4ad66 into swiftlang:main Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compnerd compnerd compnerd left review comments

@neonichu neonichu neonichu approved these changes

@abertelrud abertelrud abertelrud approved these changes

@aciidgh aciidgh Awaiting requested review from aciidgh

@friedbunny friedbunny Awaiting requested review from friedbunny

Assignees

@tomerd tomerd

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tomerd @compnerd @neonichu @abertelrud