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

declare work item for concurrent read of stderr as a non-optional #67

Closed

Conversation

sohocoke
Copy link
Contributor

To fix a warning from xcode thread sanitizer.

@kareman
Copy link
Owner

kareman commented Sep 14, 2018

Hi, thanks for pointing out this issue. But even after applying your fix, Xcode (10 GM) still complains about a data race in line 158 (self._stderror = _stderror) when running unit tests with the thread sanitiser enabled. Does your fix remove a warning when using SwiftShell from your own code?

@sohocoke
Copy link
Contributor Author

sohocoke commented Sep 15, 2018

Hmmf, just found the warning appears when I exercise a more exhaustive code path that invokes SwiftShell. I had first thought the error was because the thread sanitiser didn't fully grok #wait called on an optional, but it seems optionality was immaterial. So, this PR doesn't really fix the issue.

It would be preferable to replace all the work in RunOutput#init(launch:) -- launching the task and obtaining the output -- with an asynchronous method.

  • Stdout / stderror can be asynchronously delivered to a closure after being read, resulting in no potential race conditions to even think of.
  • It would be an opportunity to resolve the awkwardness of having a process launch on the back of creating a RunOutput object.
  • It would make instantiating RunOutput and the launching of the task safe to perform on the main thread / queue; currently it wouldn't be wise to do so, since anything that blocks or delays the concurrently performed work to process stderr, can end up blocking the main thread and give us a SPOD.

@kareman
Copy link
Owner

kareman commented Sep 17, 2018

I’m afraid I don’t quite understand what you are suggesting here. I’d love to see a PR of it if you have the time.

@sohocoke
Copy link
Contributor Author

@kareman Please see PR #69 for an implementation of the previous suggestion.

@kareman
Copy link
Owner

kareman commented Sep 19, 2018

Is continued in PR #69

@kareman kareman closed this Sep 19, 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