Fix NSTask race condition #90
Conversation
| outPipe.fileHandleForReading.readabilityHandler = nil; | ||
| errPipe.fileHandleForReading.readabilityHandler = nil; |
There was a problem hiding this comment.
I originally had these in the blocks themselves, but moved it out to avoid retain cycles. But I just saw this comment in that stackoverflow page:
However, from testing, it looks like if you avoid -availableData and instead use -readDataOfLength in the handler block, you will eventually get a call to that block with zero-length data. If you don't nil the handler at that point, you'll get called with repeated zero-length data objects. That may be the intention, it's just not documented.
I wonder if it's worth moving these back into readabilityHandler blocks (I think we will need to create weak references to the pipes)
There was a problem hiding this comment.
This makes more sense to me:
self.standardOutput = nil
self.standardError = nil
WDYT?
Then I expect the entire pipes to be released after exiting the method.
There was a problem hiding this comment.
Yeah that makes sense. based on the stackoverflow comment, I'm just thinking about what could go wrong if the readabilityHandler gets called multiple times with no data. Maybe I should create separate semaphores for stdout and stderr in case it gets dispatched twice?
There was a problem hiding this comment.
This makes more sense to me:
self.standardOutput = nil self.standardError = nilWDYT?
Then I expect the entire pipes to be released after exiting the method.
This actually doesn't work. Apparently this method raises NSInvalidArgumentException once this has been set https://developer.apple.com/documentation/foundation/nstask/1407627-standardoutput?language=objc
There was a problem hiding this comment.
So yeah, let's put the ...readabilityHandler = nil; inside the blocks (using a weak reference of each pipe) as you suggested in the first place.
There was a problem hiding this comment.
Although I'm not sure if it really matters here..
There was a problem hiding this comment.
Especially now that we have two separated semaphores 🤔
There was a problem hiding this comment.
with setting the handler to nil in the block, we can make sure there are no extra calls to the handler (which makes it more technically correct), but it would not really have any effect on the outcome and the code is slightly messier. i'll leave it up to you 😄
There was a problem hiding this comment.
@arthuralee thanks for this AMAZING contribution and the excellent investigation!
| [self launch]; | ||
|
|
||
| dispatch_semaphore_wait(waitForTermination, DISPATCH_TIME_FOREVER); | ||
| // Wait twice, once for stdout and once for stderr | ||
| dispatch_semaphore_wait(waitForOutput, DISPATCH_TIME_FOREVER); | ||
| dispatch_semaphore_wait(waitForOutput, DISPATCH_TIME_FOREVER); | ||
| // also wait for the NSTask to terminate, otherwise we can't read terminationStatus | ||
| dispatch_semaphore_wait(waitForTaskTermination, DISPATCH_TIME_FOREVER); | ||
|
|
||
| outPipe.fileHandleForReading.readabilityHandler = nil; | ||
| errPipe.fileHandleForReading.readabilityHandler = nil; | ||
|
|
||
| NSString* stdOutStr = [[[NSString alloc] initWithData:outData encoding:NSUTF8StringEncoding] stringByTrimmingCharactersInSet:NSCharacterSet.whitespaceAndNewlineCharacterSet]; |
There was a problem hiding this comment.
Excuse my OCD, please fix the indentations (new lines are not aligned) 😅
There was a problem hiding this comment.
Ah due to my tab settings, they line up. will fix!
| outPipe.fileHandleForReading.readabilityHandler = nil; | ||
| errPipe.fileHandleForReading.readabilityHandler = nil; |
There was a problem hiding this comment.
This makes more sense to me:
self.standardOutput = nil
self.standardError = nil
WDYT?
Then I expect the entire pipes to be released after exiting the method.
|
Thanks for the quick responses! I think we might be able to close quite a few issues in this repo |
|
Any chance we can get a new version release some time soon? 🙂 |
|
Of course, I’ll release a new version in a few hours (tomorrow in my timezone:smile:). |
|
@arthuralee AppleSimulatorUtils v0.9.5 released (https://github.com/wix/AppleSimulatorUtils/releases/tag/0.9.5). |
Fixes #88, fixes #89
According to https://stackoverflow.com/questions/49184623/nstask-race-condition-with-readabilityhandler-block, there seems to be a possible race condition that causes the NSTask termination handler to fire before the data is finished being read. Additionally, there seems to be some issues with reading from
fileHandle.availableDatadirectly.This PR implements the two fixes suggested by the stackoverflow post, and subsequently fixes #88 and #89 from my personal testing (I was able to reliably repro the first issue at least)