Conversation
|
I believe that on Windows, you simply have to close the process handle as soon as the process is created to effectively "detach" the process. (This is not the same as a detached console window.) Can this be implemented as a |
I believe so. This is another reason why I did not want to use Pid class. Process handle is part of Pid class on Windows. Closing it will make osHandle be able to be null. So users will need to check for osHandle not being null if they want to use it. That differs from the current situation when osHandle is always not null. And it's also inconsistent with Posix, where osHandle is same as pid and therefore always valid. Do you think it is ok? |
I think it's more of not doing a thing that doesn't make sense to do.
If you spawn a detached process, technically there is no guarantee that the PID remains valid for any amount of time - the process may exit on its own and the OS may assign the PID to another process. So, if we are to reuse |
Why would kill require owned is true? You can kill any process via task manager or "kill" program which are clearly do not own this process. |
|
Because it might not be the process you actually intend to kill - that process may have exited a long time ago. |
|
Got it. |
|
#5086 had more problems than that, though (killing processes being an intrinsically platform-dependent operation; needing to call |
I very much agree with this. |
|
Added Pid.owned property and Config.detached constant. |
CyberShadow
left a comment
CyberShadow
left a comment
There was a problem hiding this comment.
Would like to review the implementation after the refactorings are undone.
std/process.d
Outdated
| version(Posix) | ||
| { | ||
| private: | ||
| const(char)[] getExecutablePath(const(char)[] name) |
There was a problem hiding this comment.
Since there is no longer an overload set, these refactorings aren't needed any more, right? If so, can they be undone?
|
|
||
| /** | ||
| Spawn process in detached state. This removes the need in calling | ||
| $(LREF wait) to clean up the process resources. |
There was a problem hiding this comment.
Should have a note here that calling wait or kill with the resulting Pid is invalid.
std/process.d
Outdated
| This is $(D true) if process was spawned without $(LREF Config.detached), $(D false) | ||
| otherwise. You can't $(LREF wait) for or $(LREF kill) process you don't own. | ||
| */ | ||
| @property bool owned() const @safe pure nothrow |
There was a problem hiding this comment.
No need to make this public. I can't think of many cases that programs would mix owned and non-owned Pids and require distinguishing them. Making it an implementation detail also allows changing the underlying implementation in the future, if necessary.
std/process.d
Outdated
| int performWait(bool block) @trusted | ||
| { | ||
| import std.exception : enforceEx; | ||
| enforceEx!ProcessException(_owned, "Attempt to wait for the not owned process"); |
There was a problem hiding this comment.
Consider "Can't wait on a detached process"
std/process.d
Outdated
| int performWait(bool block) @trusted | ||
| { | ||
| import std.exception : enforceEx; | ||
| enforceEx!ProcessException(_owned, "Attempt to wait for the not owned process"); |
std/process.d
Outdated
|
|
||
| Throws: | ||
| $(LREF ProcessException) on failure. | ||
| $(LREF ProcessException) on failure or on attempt to wait for not owned process. |
There was a problem hiding this comment.
Consider "detached" or "non-child" instead of "not owned", the "owned" term should be an implementation detail
std/process.d
Outdated
|
|
||
| Throws: | ||
| $(LREF ProcessException) on failure. | ||
| $(LREF ProcessException) on failure or on attempt to wait for not owned process. |
std/process.d
Outdated
|
|
||
| Throws: | ||
| $(LREF ProcessException) on error (e.g. if codeOrSignal is invalid). | ||
| or on attempt to kill not owned process. |
std/process.d
Outdated
| void kill(Pid pid, int codeOrSignal) | ||
| { | ||
| import std.exception : enforceEx; | ||
| enforceEx!ProcessException(pid.owned(), "Attempt to kill not owned process"); |
Is this an expected and unavoidable effect of the unit test, or do you need time/assistance in tracking it down? |
It should not happen. It's simple TestScript usage. But I can't understand why it happens. |
|
Ok, I think I got it. TestScript destructor runs (effectively removing the file) before file execution. Looks like race condition between /bin/sh running with script file and removing this file by unittest. |
FreeSlave
force-pushed
the
spawnProcessDetached
branch
2 times, most recently
from
45c0286 to
827dcd5
Compare
| else | ||
| throw ProcessException.newFromErrno("Could not create pipe to get process pid"); | ||
| } | ||
| scope(exit) if (config & Config.detached) close(pidPipe[0]); |
There was a problem hiding this comment.
Any reason to not reuse the same pipe used for communicating the result (forkPipe)?
There was a problem hiding this comment.
After the second fork started the first and the second forks run in parallel. I can't ensure that pid will be written to the pipe before possible errors.
There was a problem hiding this comment.
I see. Please add this explanation as a comment somewhere.
There was a problem hiding this comment.
I guess calling getpid from the second child is also a possibility, but I guess it's not worth the change.
There was a problem hiding this comment.
There's also a possibility that the second fork fails. In this case we would need to pass fictitious pid from the first fork to adjust.
There was a problem hiding this comment.
make these considerations a nice comment
There was a problem hiding this comment.
Already considered in the comment before int[2] pidPipe;
| close(workDirFD); | ||
| } | ||
| close(workDirFD); | ||
| } |
There was a problem hiding this comment.
Why change the working directory in the second fork? Wouldn't it be simpler to change it in the first fork regardless of Config.detached?
std/process.d
Outdated
| pfds[i].fd = i + 3; | ||
| pfds[i].events = 0; | ||
| pfds[i].revents = 0; | ||
| setsid(); |
There was a problem hiding this comment.
Hmm, isn't setsid supposed to be called from the first fork (so that the session leader is killed)?
There was a problem hiding this comment.
You're right. Will change.
There was a problem hiding this comment.
I found that setsid makes some apps run incorrectly when starting from terminal without setting stdin to /dev/null.
E.g. run setsid openarena from terminal. It shows a window, but does render anything.
Should we do anything about that?
There was a problem hiding this comment.
Unfortunately I am not sufficiently familiar with the subject to confidently answer that.
From looking online, I noticed that setsid is something that is often done when double-forking, but not prescriptively so. Perhaps it only makes sense for non-interactive daemon processes?
There was a problem hiding this comment.
If we don't setsid, the spawned process will belong to the same session so pkill -s '$sid' will send signal to this process too.
However on my system all my desktop applications belong to the same session, so it's ok.
We can add another flag to config to control whether the forked child should call setsid.
There was a problem hiding this comment.
OK, so correct me if I'm wrong, but as I understand the most commonly-observed difference will be that ^C will not kill the spawned process if it's in the same session.
In which case, it does sound like it's worthy of a Flag addition, with the Windows implementation being mapped to the CREATE_NEW_PROCESS_GROUP process creation flag.
There was a problem hiding this comment.
It may be worth leaving it for a separate PR.
There was a problem hiding this comment.
^C is sent only to the foreground processes of session. And session has distinction between foreground and background processes only if it has connection to controlling terminal.
Even if doing setsid via flag it's still not clear if user want the new process to be a session leader (i.e. call setsid in the second fork) or not (call setsid in the first fork and let it die).
It may be worth leaving it for a separate PR.
So shall I remove setsid for now?
There was a problem hiding this comment.
Thanks for the clarification. I think I'm out of my depth here. Perhaps someone more knowledgeable than me about POSIX process sessions can chime in, @kyllingstad @schveiguy @jmdavis ?
There was a problem hiding this comment.
I've done some experiments. Login via ssh (just plain login without tmux, screen or similar) and spawned some script process in detached state without calling to setsid. Logout and re-login again, the process still ran. So I guess setsid is not necessary after all.
std/process.d
Outdated
| } | ||
|
|
||
| private: | ||
| @property bool owned() const @safe pure nothrow |
There was a problem hiding this comment.
No point in a private property, just name the field owned.
std/process.d
Outdated
| This leads to the annoying message like "/bin/sh: 0: Can't open /tmp/std.process temporary file" to appear when running tests. | ||
| It does not happen in unittests with non-detached processes because we always wait() for them to finish. | ||
| */ | ||
| Thread.sleep(dur!"msecs"(5)); |
There was a problem hiding this comment.
I think this is fine, but I'd give it a more generous timeout of 100 msecs or so just to exclude race conditions.
There was a problem hiding this comment.
I used 5 msecs, because there's the same sleep in the other unittest under version(Android), having a comment about Android killing sleeping applications.
There was a problem hiding this comment.
That's fine, we don't have CI for Android; I'm more concerned about Windows, where process creation is very slow. If the Android maintainer finds that this test is causing them trouble then they can fix it as needed. (Also, I was pretty sure Android kills idle apps, not processes...)
|
Thanks for your pull request, @FreeSlave! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Some tips to help speed things up:
Bear in mind that large or tricky changes may require multiple rounds of review and revision. Please see CONTRIBUTING.md for more information. Bugzilla references
|
|
Could you please rebase on top of the latest master? That should fix CircleCI. |
FreeSlave
force-pushed
the
spawnProcessDetached
branch
from
07578c0 to
fb3ff62
Compare
|
Rebased. Is any other change needed? |
|
Ah, a changelog entry or linked bug as per @dlang-bot's comment. I think that's all if there is no further feedback. |
…ge directory in the first fork
|
I've added changelog FreeSlave@fa8a73f, but github does not see the changes. Probably because of the previous rebase. |
FreeSlave
force-pushed
the
spawnProcessDetached
branch
from
2a32acd to
fa8a73f
Compare
|
@wilzbach Any idea why changelog is not showing up in DAutoTest diff here? |
Yes, it seems like Martin has added this file and thus it doesn't get generated: https://github.com/dlang/dlang.org/blob/master/changelog/2.075.0_pre.dd Should we bump the version at dlang.org? Probably a good idea to do this for all version files after the master -> stable branch-off? |
Sounds like a good idea. |
|
|
||
| Config.detached allows to $(REF_ALTTEXT spawn process, spawnProcess, std, process) independently from the caller. | ||
| No need to wait on the process spawned that way - no zombie will left after process exits. | ||
| Attempts to $(REF wait, std, process) on or $(REF kill, std, process) detached processes will throw. |
There was a problem hiding this comment.
Some copyediting:
Config.detached allows $(REF_ALTTEXT spawning processes, spawnProcess, std, process) which run independently from the current process. There is no need to wait on the processes created with this flag, and no $(HTTPS en.wikipedia.org/wiki/Zombie_process, zombie process) will be left after they exit.
Attempts to call
|
Does this really need Andrei's approval? It doesn't really add a new symbol. |
Well there's
|
std/process.d
Outdated
| chdir, | ||
| getrlimit | ||
| getrlimit, | ||
| doubleFork |
There was a problem hiding this comment.
add a comma so the next addition blames properly
std/process.d
Outdated
| // but we don't have a direct access to the second fork pid from the caller side thus use a pipe. | ||
| // We also can't reuse forkPipe for that purpose | ||
| // because we can't predict the order in which pid and possible error will be written | ||
| // since the first and the second forks will run in parallel. |
There was a problem hiding this comment.
looks like a multiline comment
| if (core.sys.posix.unistd.pipe(pidPipe) == 0) | ||
| setCLOEXEC(pidPipe[1], true); | ||
| else | ||
| throw ProcessException.newFromErrno("Could not create pipe to get process pid"); |
There was a problem hiding this comment.
Simplify flow:
if (core.sys.posix.unistd.pipe(pidPipe) != 0)
throw ...;
setCLOEXEC(pidPipe[1], true);| else | ||
| throw ProcessException.newFromErrno("Could not create pipe to get process pid"); | ||
| } | ||
| scope(exit) if (config & Config.detached) close(pidPipe[0]); |
There was a problem hiding this comment.
make these considerations a nice comment
std/process.d
Outdated
| // In the case that stderr is redirected to stdout, we need | ||
| // to backup the file descriptor since stdout may be redirected | ||
| // as well. | ||
| if (stderrFD == STDOUT_FILENO) stderrFD = dup(stderrFD); |
There was a problem hiding this comment.
It was like this in previous implementation
std/process.d
Outdated
| immutable maxToClose = maxDescriptors - 3; | ||
|
|
||
| // Call poll() to see which ones are actually open: | ||
| pollfd* pfds = cast(pollfd*) malloc(pollfd.sizeof * maxToClose); |
There was a problem hiding this comment.
Again, you're asking to change things that were like this before me. Well, I can, for sure.
std/process.d
Outdated
| auto status = InternalError.noerror; | ||
| auto readExecResult = core.sys.posix.unistd.read(forkPipe[0], &status, status.sizeof); | ||
| // Save error number just in case if subsequent "waitpid" fails and overrides errno | ||
| auto lastError = .errno; |
std/process.d
Outdated
|
|
||
| if (readExecResult == error.sizeof) | ||
| throw ProcessException.newFromErrno(error, errorMsg); | ||
| else |
| if (read(pidPipe[0], &actualPid, id.sizeof) == id.sizeof) | ||
| id = actualPid; | ||
| else | ||
| throw ProcessException.newFromErrno("Could not read from pipe to get detached process id"); |
std/process.d
Outdated
| CloseHandle(pi.hProcess); | ||
| return new Pid(pi.dwProcessId); | ||
| } | ||
| else |
There was a problem hiding this comment.
Delete what? Can you elaborate? Oh, you mean just else. Got it.
Please let me be the point of failure for the sake of better checks and balances of library consistency. In several prior cases it has happened that design sensibilities somewhat foreign to D/Phobos have entered the standard library, to everybody's displeasure. Thanks. |
schveiguy
left a comment
schveiguy
left a comment
There was a problem hiding this comment.
LGTM, I would just tweak the timing a bit. There's no guarantee on how long a process takes to finish, especially on a busy OS, and losing a few seconds in the time to test phobos isn't going to kill us.
std/process.d
Outdated
| CloseHandle(pi.hProcess); | ||
| return new Pid(pi.dwProcessId); | ||
| } | ||
| else |
std/process.d
Outdated
| wait(pid); | ||
| else | ||
| // We need to wait a little to ensure that the process has finished and data was written to files | ||
| Thread.sleep(dur!"msecs"(200)); |
There was a problem hiding this comment.
Wait longer. There's no need to vie for performance here. Wait 2 seconds instead of .2
There was a problem hiding this comment.
Also, stylistic change, you can put 2.seconds instead of dur!"seconds"(2)
There was a problem hiding this comment.
I would prefer unittests run as fast as they could. But ok.
std/process.d
Outdated
| This leads to the annoying message like "/bin/sh: 0: Can't open /tmp/std.process temporary file" to appear when running tests. | ||
| It does not happen in unittests with non-detached processes because we always wait() for them to finish. | ||
| */ | ||
| Thread.sleep(dur!"msecs"(100)); |
andralex
left a comment
andralex
left a comment
There was a problem hiding this comment.
I'm ok with the addition, leaving detailed reviews to @schveiguy and others
|
ping @CyberShadow any idea why the DAutoTest fails? |
@wilzbach this looks invalid to me, this has nothing to do with RedBlackTree. I don't actually see a bugzilla reference in any commit, so not sure why this showed up. |
Please see #5639 (comment) / #5626 (comment) / #5625 (comment) / #5619 (comment) / #5617 (comment). TL;DR: GitHub bug. |
|
@CyberShadow thanks, here's a followup: while this bug exists, what is the correct response, seeing as that test is required for merging? Should I just ping you/@wilzbach, or is there a way I can fix it myself? |
AFAICT the only safe way is to create a new PR. We can overwrite it manually if the auto-tester passes, but there is always the slight risk of breaking Jenkins or CircleCI though. However, at least for CircleCi we get a warning mail afterwards, so I think we can start to risk it. Creating a new PR all the time, is really annoying. |
I would guess that it's due to the GitHub API sending rubbish. |
Tried it this time - let's hope for the best. We really need to get GitHub to fix their bugs though.. |
8 participants
spawnProcessDetached is function similar to spawnProcess, but user does not need to wait on spawned process to avoid zombies (resource leak). Instead of returning Pid instance spawnProcessDetached returns integer pid via pointer. Rationale: user should not be able to wait on detached process.
Currently only Posix version is implemented. It uses double fork technique and setsid to detach the new process from its caller. Since this implementation reuses a lot of stuff from spawnProcess I moved some implementation details to separate private functions to share them between spawnProcess and spawnProcessDetached.
THIS IS WIP
I'll add more unittests and Windows version later. For now say if you like the idea.