-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
[Utility] Cleaned up api for ProcessSet. #942
[Utility] Cleaned up api for ProcessSet. #942
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thanks!
Minor comments
Sources/Utility/ProcessSet.swift
Outdated
thread.join() | ||
// Wait until all processes terminate | ||
for process in self.processes { | ||
try process.waitUntilExit() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this try?
and this method as non throwing
@@ -70,7 +69,7 @@ class ProcessSetTests: XCTestCase { | |||
threadStartCondition.wait() | |||
} | |||
} | |||
processSet.terminate() | |||
try processSet.terminate() | |||
|
|||
t.join() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you see time taken by the test testSigInt
before and after waitUntilAfter() in process set? There should be a noticeable difference I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actually notice any major difference. 😕
994acee
to
c78c674
Compare
Sources/Utility/ProcessSet.swift
Outdated
/// Terminate all the processes. This method blocks until all processes in the set are terminated. | ||
/// | ||
/// A process set cannot be used once it has been asked to terminate. | ||
/// Throws: If an error occurs during the wait for a process to terminate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true anymore
Clients are no longer required to call remove on a process set.
c78c674
to
f52ca43
Compare
@swift-ci please test |
I opened #951 with a small addition to this patch Thanks! |
Clients are no longer required to call remove on a process set.