run cleanupRelay only once in TerminalIO and StandardIO#153
run cleanupRelay only once in TerminalIO and StandardIO#153crosbymichael merged 10 commits intoapple:mainfrom
Conversation
|
@adityaramani I think the formatting issues have been fixed |
|
Im gonna pull in the changes locally and test it out as well |
|
thank you, I've been depending on the workflow tests and it's not fair to you that they're not passing |
|
No worries at all! |
|
Tried running the tests in a loop and it failed with this, and there werent any other logs from the guest I'll try combining your change with what I was testing out with the |
|
dang, I really thought that was going to solve it. I'm going to look at this again when I get home, I got caught in a literal sandpit 💀 |
|
Thanks! I think your changes look good - and I tested it after adding in a couple of other fixes (see here https://github.com/adityaramani/containerization/tree/vminitd-crash) Although I do think there maybe some other issues in the process handling flow, this change makes it more stable. Will wait for the others to review |
|
@adityaramani do you want me to cherry pick your commit? it's only partially signed so I don't know |
|
Yeah go for it. Take that commit in. If the signature are a problem just apply the diff |
|
ok done, but it says unverified |
|
Yeah. Maybe try removing me as author? |
Signed-off-by: Aditya Ramani <a_ramani@apple.com>
|
@adityaramani if I could ask, what are the specs of the machine you're testing on? |
|
It's an M2 Pro with 64 gb of ram. Are you asking cause you don't see the issue on your end? I need to run the test on a loop and see it failing once in like 15 times |
|
I was just wondering, I think it has to be a race condition then. increasing system load ( |
| for proc in processes { | ||
| let pid = proc.pid | ||
| self.log?.debug("checking for exit of managed process", metadata: ["pid": "\(pid)", "exits": "\(exited)"]) | ||
| self.log?.debug("checking for exit of managed process", metadata: ["exits": "\(exited)", "processes": "\(processes.count)"]) |
There was a problem hiding this comment.
This looks good to me (and sorry for delay), but I'll wait for either @adityaramani or @crosbymichael to give it a second thumbs up.
|
LGTM! I think Michael should take a look as well |
possibly fixes #124