Skip to content
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

Async command launch #69

Merged
merged 3 commits into from
Sep 26, 2018
Merged

Conversation

sohocoke
Copy link
Contributor

It is undesirable to keep the task launch operation as a synchronous side-effect of RunOutput#init, since:

  1. Xcode thread sanitiser flags a warning for a race condition for the _stderr variable;
  2. It will block until the concurrent read to stderr finishes, making RunOutput inappropriate to initialise on a Cocoa app's main thread.

In this PR, RunOutput's initialiser has been turned 'dumb', all mutation eliminated, and the task launch operation relocated to AsyncCommand#launch(complete: @escaping (RunOutput) -> Void)).

Further points:

  • To avoid changing the API, CommandRunning#run(_ executable...), which uses AsyncCommand, remains synchronous and waits for #launch to complete, taking on issue 2 above.
  • AsyncCommand#launch is a standard asynchronous method, so it makes the presence of PrintedAsyncCommand#onCompletion, which is a completion handler setter, a little awkward.

@kareman
Copy link
Owner

kareman commented Sep 18, 2018

This is great! Thank you.

My only request is that you make AsyncCommand#launch fileprivate, as I don't see any use for it outside of the run command. The only way a user can get hold of an AsyncCommand instance is with the runAsync command, and then it is always already launched.

@sohocoke
Copy link
Contributor Author

Changed per previous comment.

I'd like to mention, though, that it would be better for #runAsync to be changed to call #launch, and probably to remove AsyncCommand#init(launch:...) -- as it stands I find the code paths quite confusing.

I stopped short of making these changes since I didn't quite understand the role of the arguments to #runAsync.

@kareman
Copy link
Owner

kareman commented Sep 26, 2018

Yes, the internal documentation could be made clearer. I will try to improve it before I release the new version for Swift 4.2.

@kareman kareman merged commit 32e5d5a into kareman:master Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants