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
Move iTerm-server daemon to per-user namespace. Issue 4147 #381
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Learned a bunch about macOS, thanks! Pointed out a few spelling/grammar changes. Woot
sources/shell_launcher.c
Outdated
|
||
Issue 4147 happens because the forked daemon is running in the Aqua session namespace. So | ||
when the user logout or a GUI crash happens, the forked daemon remains isolated in a lost | ||
per-session namespace. It partially looses it's IPC capabilities to comunicate with other |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partially loses its ability ... to communicate ...
sources/shell_launcher.c
Outdated
namespace that already exists [the current one], or any new 'per-session' namespaces that | ||
may in future be (re)created below it. | ||
|
||
Important is to mention that it's availability is to serve just the same user (the same UID). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that its availability ...
sources/shell_launcher.c
Outdated
Important is to mention that it's availability is to serve just the same user (the same UID). | ||
This is what we want, because all other BSD preparations here is done to create the | ||
iTerm-Server to serve just that given user who started it. If another user is logged-in via | ||
GUI via fast user switching, this other user will have it's own Per-User session, and will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have its own ...
sources/shell_launcher.c
Outdated
This is what we want, because all other BSD preparations here is done to create the | ||
iTerm-Server to serve just that given user who started it. If another user is logged-in via | ||
GUI via fast user switching, this other user will have it's own Per-User session, and will | ||
have below it his Aqua per-session GUI belonging to it's UID, and his programs running there, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to its UID ...
sources/shell_launcher.c
Outdated
iTerm-Server to serve just that given user who started it. If another user is logged-in via | ||
GUI via fast user switching, this other user will have it's own Per-User session, and will | ||
have below it his Aqua per-session GUI belonging to it's UID, and his programs running there, | ||
including his own iTerm-Server daemons forked just for it's UID use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for its UID ...
sources/shell_launcher.c
Outdated
is never destroyed by GUI logout or crashes. And it will exist until there is at least one | ||
daemon or service running on it, for that given user. [* from Apples documentation] | ||
|
||
That's the exact namespace where Launchd places daemons for a given user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Great writeup and docs!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
sources/shell_launcher.c
Outdated
return; /* All Done! now return, as we are ready to begin forking! */ | ||
} | ||
// Never reach. Unless if we had failed to get the parent bootstrap port, which according | ||
// to Apple's documentation never happens. But I already saw rare wierd system situations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saw weird system ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the spelling/grammar changes!
My natural language is Portuguese (I'm from Brazil), some (sometimes a lot of) mistakes happens, :)
sources/shell_launcher.c
Outdated
// to Apple's documentation never happens. But I already saw rare wierd system situations | ||
// where it may happen. | ||
// Almost impossible, but if it happens, for sure is a BUG. So log it just in case. | ||
FDLog(LOG_ERR,"BUG: Error getting parent BS port! Please report this msg and code: '%x'", kr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a fan of defensive error logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me too, learned the importance of that on my last 9 years working as developer on the asterisk telephony platform. There are a lot of similar BUG situations on that code, and naturally I created this function the way I'm accustomed to do.
Thank you for your appreciation!
This is a fantastic PR! Thank you! |
Thank you for creating iTerm2 @gnachman, you deserve the best congratulations! |
If you want to TEST what kind of session you are, I wrote this simple code that will show the current session you are. I needed to do it during the validation/debug phase of my PR. Here is a simplified version of it. compile using: CODE: which_namespace.c
. You can use it to test iTerm2 before and after this PR patch. This is the output before:
This is the output after this PR patch.
|
Awesome! How did you figure this out? |
Here is all the information sources I used to achieve this PR . Apple Excellent technical note about the intrinsic mechanism of BSD + Mach kernel portions Apple Libc source code MacOS launchctl source code MacOS 10.14 kernel fork() source code Additional researches looking for use cases at : MacOS 10.14.0 Kernel source code Various system packages source code for Mojave 10.14.1 that deals with process execution |
This PR fixes issue 4147