fix: data race running many processes concurrently#167
Conversation
This test is flaky rather than always failing, but with 1000 runs it's sufficiently flaky to be useful.
There are lots of potential races when reading from process file handles. This approach extracts the handling into the file handle itself, which makes it easier to get the lifecycle right. This also unifies reading from the file handles into one place, as we were losing data between the readabilityHandler and sync readToEnd calls.
|
Hey @waltflanagan, any thoughts on this? |
|
@danpalmer thanks for the tag, i'll try to find some time to take a look over the next day or two. This is tricky stuff, thanks for taking a look. |
|
This may also help if you haven't come across it yet. https://developer.apple.com/forums/thread/690310 It does mention needing to dispatch to main in the termination handler to avoid a race in the IO handlers. I wonder if we ended up falling into that. |
I didn't know about this one. I'd say let's merge this work, and if the issue still manifests after, we can mimic the implementation suggested in that thread. I have to say it's insane the amount of dancing it's required to get that to work. In other standard libraries, it's much more straightforward. On another note, there's also subprocess coming to
That's completely fine. As long as it does the job respecting the contract of the API, how it's implemented internally is not that important.
Please, go ahead with those. I'll add you as a collaborator of the repository so that you don't have to work from forks, which is slightly more inconvenient. |
| import Testing | ||
| @testable import Command | ||
|
|
||
| struct CommandRunnerRaceTests { |
There was a problem hiding this comment.
@danpalmer I took the opportunity to adjust your test to use Swift Testing instead of XCTest, which we are trying to use less and less.
|
@all-contributors add @danpalmer for code |
|
I've put up a pull request to add @danpalmer! 🎉 |
|
Command 0.11.18 is available with these changes: https://github.com/tuist/Command/releases/tag/0.11.18 |
This PR is an attempt at fixing #160. A test is included which attempts to exercise this case. It typically fails when running before the fix, and appears to pass every time after the fix.
I realise this is a bit of a departure on the code, very open to feedback on this, although I'd ask that you give any non-style changes a go locally, as this took a lot of trial and error to get to and lots of changes that one might expect would work, actually won't.
During development of this I found some other issues where similar things can happen (reading from closed file handles) in the
lookupExecutablecall, as this isn't as strict as the main command running. I'll attempt to tackle that separately, and haven't managed to reproduce that in a test yet.