Partial Fix Issue 16615 - convert os pid to std.process Pid by edi33416 · Pull Request #5086 · dlang/phobos

edi33416 · 2017-02-03T23:24:24Z

The issue on bugzilla states that we are currently missing the functionality of converting a pid to a std.process Pid in order to be able to use the kill function, if someone were to implement a task manager for example.

dlang-bot · 2017-02-03T23:24:26Z

Fix	Bugzilla	Description
✗	16615	std.process is missing functionality for child processes

JackStouffer · 2017-02-04T06:38:58Z

While this is a nice convenience function, this does not address the primary complaint of the bug report: an inability to easily get the pid of a child process.

Please change your commit message to Partial Fix For Issue 16615 - convert os pid to std.process Pid

DmitryOlshansky · 2017-02-08T12:34:59Z

std/process.d

+{
+    version (Windows)    TestScript prog = "exit %~1";
+    else version (Posix) TestScript prog = "exit $1";
+    auto pid = spawnProcess([prog.path, "10"]);


This is racy - by the time you issue kill the process may already exit normally. An infinite loop would be a better choice then "exit $1"

DmitryOlshansky · 2017-02-08T12:36:12Z

std/process.d

+                _processHandle = new Pid(pid, _handle);
+            }
+
+            return _processHandle;


Most functions of this module will throw on error, I suggest to do the same here.

andralex

Please fix the nits

andralex · 2017-06-07T20:30:31Z

std/process.d

+    version (Windows)
+    {
+        /**
+        A simple factory that returns a $(LREF Pid) handle when provided with a


LREF is deprecated, please use REF e.g. $(REF Pid, std, process).

That would be new to me.
LREF = local reference (within the module), only XREF was deprecated.

andralex · 2017-06-07T20:30:55Z

std/process.d

+        A simple factory that returns a $(LREF Pid) handle when provided with a
+        global `pid`. This handle can be used to kill processes that haven't been
+        spawned by this process.
+        `Note:` this does not check if the provided `pid` is valid and corresponds


Note should not be in code font.

s/if/whether/

andralex · 2017-06-07T20:35:10Z

std/process.d

+
+        (Windows specific) Throws:
+        $(LREF ProcessException) on failure.
+        */


FYI we currently only generate documentation on Posix, so this doc won't appear anywhere. At some point we should get things going by running wine to build windows dox...

andralex · 2017-06-07T21:35:00Z

std/process.d

+            pid = a process id
+
+        Returns: an instance to a `Pid` that corresponds to the pid within the
+        operating system, or, on Windows, throws `ProcessException` if opening the


The info about throwing should be in a separate section:

Throws: this and that and the other.

Removed the throws part all together since it is Windows specific.

CyberShadow

Please amend.

CyberShadow · 2017-06-08T12:53:10Z

std/process.d


+@system unittest // Pid.fromSystemPid()
+{
+    version (Windows)    TestScript prog = "do { timeout 1 } while (1)";


What the heck is this syntax? It's definitely not .cmd syntax. Is this a broken test?

It's powershell syntax

Well, it's not being executed as a PowerShell script. Look at the implementation of TestScript. The string is saved to a .cmd file. In addition to that, I don't think you can just CreateProcess / spawnProcess a .cmd etc. file on Windows. Windows does not have shebangs, associations (incl. .cmd) are handled by userspace (the shell) in a layer above CreateProcess, so you must spawn the command interpreter explicitly.

In addition to fixing the script, you need to find why your test was broken (in that it allowed a broken script to work and pass the test).

Well, it's not being executed as a PowerShell script. Look at the implementation of TestScript. The string is saved to a .cmd file. [..]

I have missed that. I just assumed that if the syntax was invalid, auto tester would complain about it.

In addition to fixing the script, you need to find why your test was broken (in that it allowed a broken script to work and pass the test).

Again, I just assumed everything was ok since the tester did not complain about it.
Any suggestions on how to approach the issue and find out why the tester is not complaining?

Any suggestions on how to approach the issue and find out why the tester is not complaining?

You need to find why your test is broken. You might be missing a check which ensures that everything has gone as expected. If you need more time for this, I suggest we revert the PR for now.

Try inserting Thread.sleep(1.seconds) before the kill. I'm guessing your test is killing the process before the OS figures out the test script is invalid, in which case it's at least a race condition.

Try inserting Thread.sleep(1.seconds) before the kill. I'm guessing your test is killing the process before the OS figures out the test script is invalid, in which case it's at least a race condition.

Great observation! I totally missed that this could happen.

CyberShadow · 2017-06-08T12:54:07Z

std/process.d

+
+        Throws: $(LREF ProcessException) if opening the process failed; this is
+        Windows specific.
+        */


Do not have different documentation blocks for different platforms. Most users will never see the Windows one. Instead, have a look at version(StdDDoc).

(or just move the version blocks from outside the function body to inside it, so there's only one declaration)

CyberShadow · 2017-06-08T12:55:03Z

std/process.d

+
+        Throws: $(LREF ProcessException) if opening the process failed; this is
+        Windows specific.
+        */


This, or the wait documentation, should mention that wait-ing on a Pid that does not belong to a child (i.e. not created by spawnProcess etc.) is invalid and is not going to work.

Actually, it seems like there's more to it - currently wait uses asserts to verify the validity of the Pid. If non-child PIDs becomes part of its API, I think those checks need to be made explicit.

yah that would need to change if we allow non-child pids

CyberShadow · 2017-06-08T12:56:31Z

@andralex Please ping me or Steven or Lars if you need something reviewed regarding std.file or std.process.

CyberShadow · 2017-06-08T13:20:55Z

So, question: do we actually need this in Phobos? It is certainly a simple enough addition, but:

std.process.Pid was created not to encapsulate the PID of any process, but only child processes created by spawnProcess et al. For instance, on Windows it has a _handle field which you only get when creating the process (or "opening" it manually). So, for one thing, wait(Pid) is now slightly changed in meaning: whereas before it was implied that its parameter could only represent a child process's PID, this is now no longer true, and the underlying primitive won't work with other Pids.

Another question is where do we want to draw the line for general process management. Killing arbitrary processes is the immediate benefit of this change. However, do we also want to have things such as process enumeration, renicing, reading/writing process memory etc.?

edi33416 · 2017-06-08T15:47:01Z

So, question: do we actually need this in Phobos? It is certainly a simple enough addition, but:

I think we should have this in Phobos, as, imho, this expands the functionality of std.process quite a lot: now you could write a task manager using D (right?)

std.process.Pid was created not to encapsulate the PID of any process, but only child processes created by spawnProcess et al. For instance, on Windows it has a _handle field which you only get when creating the process (or "opening" it manually). So, for one thing, wait(Pid) is now slightly changed in meaning: whereas before it was implied that its parameter could only represent a child process's PID, this is now no longer true, and the underlying primitive won't work with other Pids.

I think wait should only be allowed for processes that were created using spawnProcess, so this might need some tweaking.

Another question is where do we want to draw the line for general process management. Killing arbitrary processes is the immediate benefit of this change. However, do we also want to have things such as process enumeration, renicing, reading/writing process memory etc.?

On the top of my head, probably only wait and anonymous pipes shouldn't be allowed, since these depend on process relationship. Everything else, imo, should work; we are a systems programming language after all.

edi33416 · 2017-06-09T13:13:45Z

@CyberShadow what do you think?

CyberShadow · 2017-06-09T14:58:48Z

Everything else, imo, should work; we are a systems programming language after all.

I don't think that argument makes sense (you can say it about a lot of useless or under-designed proposals); otherwise I can review a follow-up PR, but personally I would like more opinions on whether this functionality is necessary in Phobos. As I see it allowing Pid to represent non-child processes is shoe-horning functionality into it it was never meant to have, with the _handle field being the smoking gun.

andralex · 2017-06-09T15:04:46Z

Few questions:

So now we have wait(pid) work the same for children pids, but not as one might expect for pids obtained with fromSystemPid, correct?

Is the call wait(pid) for "bad" pids unsafe/liable to cause immediate abort etc? What will be the error case - throw, error code, ...?

Do other useful functions of Pid apply to pids obtained with fromSystemPid, i.e. what are the good uses counterbalancing the bad use?

Thanks.

CyberShadow · 2017-06-09T15:07:13Z

Is the call wait(pid) for "bad" pids unsafe/liable to cause immediate abort etc?

Right now there is an assert in the Windows implementation of wait, I've requested that it's changed to an explicit check above.

Do other useful functions of Pid apply to pids obtained with fromSystemPid, i.e. what are the good uses counterbalancing the bad use?

AFAIU, only kill (which is why I'm not sure about this addtition).

andralex · 2017-06-09T15:26:31Z

So that puts as at 50/50. Assuming we add more operations on pids, are the majority of those likely to work uniformly for child and non-child processes? If so, adding is good. Are the majority of those likely to NOT work the same for child and non-child processes? Then let's not add.

Is there a possibility to fill _handle appropriately on non-child processes?

(My cringe: Pid is actually a GC class instead of a refcounted struct)

CyberShadow · 2017-06-09T15:48:26Z

Assuming we add more operations on pids

I think we need some concrete proposals of what more could be added, and I can't think of any that would belong in Phobos - as I wrote above: "However, do we also want to have things such as process enumeration, renicing, reading/writing process memory etc.?"

Is there a possibility to fill _handle appropriately on non-child processes?

Actually it looks like I misread the patch - the added function does fill it, as a handle is needed to kill a process, however for doing so it adds an optional parameter (desiredAccess). Which raises a few questions, like:

We are already straying into platform-dependent APIs in that the user would need to import the Windows API modules to use the appropriate process access flags. It's already bad enough that kill's parameter means completely different things on Windows and POSIX (exit code and signal respectively).
If the only supported operation on a non-child Pid is killing it, why even allow users to customize which access to open the process with?
By now it's already clear that Pid is a misnomer, since (esp. on Windows) it does not represent a process ID, but a process handle through which a child can be killed and waited on, but this further pushes the issue by having the Windows implementation opening a handle to the process on construction.

CyberShadow · 2017-06-09T15:51:17Z

Especially considering the problem with kill's API, and given that killing processes is the only thing you can do with a non-child PID, I'm now convinced that this is not something Phobos needs to bother with. You are going to have to go into platform-specific territory anyway with kill's parameter, so considering all the desiredAccess ugliness this adds very little value.

schveiguy · 2017-06-09T16:02:52Z

We have discussed this kind of change before. Killing a pid with the OS API is pretty simple, we could probably wrap that into a single function.

The std.process API generally is for creating processes and handling the output/results. When you tack on managing processes that you didn't create, it's almost a separate API. It would belong in std.process, but not in the existing Pid type.

Things that you can do with a process id should dictate where our OS-agnostic API should go. We can get pretty far into the weeds on Windows, but Unix capabilties are pretty straightforward (if you don't count /proc/<pid> on Linux).

A very simple addition would just be a kill function that does the right thing depending on OS. Maybe nice as well.

andralex · 2017-06-09T16:57:24Z

Actually it looks like I misread the patch

Was about to point out the same. Then why was the revert pulled before this concluded?

A larger point - how can we reduce the churn? This whole discussion ideally would have taken place in the issue.

CyberShadow · 2017-06-09T17:02:05Z

Then why was the revert pulled before this concluded?

Because it's obvious there are several non-trivial issues here, the implementation needs improvement anyway, and more time is needed to settle them.

A larger point - how can we reduce the churn? This whole discussion ideally would have taken place in the issue.

Sorry about that - I haven't seen the original issue or the PR when it was created. I've subscribed to all PRs/issues after DConf but anything that happened before then wasn't on my radar.

schveiguy · 2017-06-09T17:03:16Z

how can we reduce the churn?

Just speaking for myself -- I haven't had tons of time to read through all the bug reports, so I have lost track of which ones would be good for me to review. Also my list of unread emails from github is hovering over 4000. I just happened to click on this one when it was brought to the top in a few spare minutes I had.

The best advice is if there is a section of the runtime/library that is pertinent to a particular person, ping that person either via github in the PR or via bugzilla by subscribing them to that bug. Otherwise, sometimes life gets in the way, and that person may miss it until later. I personally don't have a lot of traffic from bugzilla, so I would see that more likely, and anything that comes from github goes into a bucket on my email except if it has @schveiguy in it, then it goes in my inbox.

CyberShadow · 2017-06-09T17:03:30Z

Was about to point out the same. Then why was the revert pulled before this concluded?

(Also, I proposed the revert specifically after I've re-read the patch, as I think the actual solution was not better than what I thought it was, and it made me realize additional problems with the proposal.)

CyberShadow · 2017-06-09T17:15:58Z

and anything that comes from github goes into a bucket on my email except if it has @schveiguy in it, then it goes in my inbox

Same here. OT:

Shortly after DConf, I realized that my morning and evening routine consisted of at least half an hour of Reddit/HN (mainly reading stuff for the sake of it), and I noticed an opportunity for optimization: I've subscribed to all GitHub and Bugzilla notifications (sending those to a separate label) and replaced part of my Reddit time with looking through what accumulated there. Gmail's interface makes it very easy to blaze through threads that are not relevant, so picking out threads relevant to you (or where you can contribute something like triaging a low-effort bug report or bisecting a regression) becomes easy as well. Hopefully I can keep up this habit going forwards, and of course there is the temporary problem that situations like this happen where something shows up on your radar only after it's been merged.

andralex · 2017-06-09T18:03:54Z

Because it's obvious there are several non-trivial issues here, the implementation needs improvement anyway, and more time is needed to settle them.

There is time until the next release.

Because it's obvious there are several non-trivial issues here, the implementation needs improvement anyway, and more time is needed to settle them.

Could you please get into those?

CyberShadow · 2017-06-09T18:05:30Z

Could you please get into those?

I did, and barring additional arguments my conclusion so far is that this is not something that should be in Phobos. See also Steven's reply on the PR - we could have a kill(int pid, int signalOrExitCode) instead, but at this point this needs a rationale of what the submitter is trying to achieve.

CyberShadow · 2017-06-09T18:08:27Z

There is time until the next release.

I think the stable branch can be forked earlier than the release for RCs and such. Either way, I think it's better to not rely on a calendar and revert early regardless (and lest the revert be forgotten or get stuck in analysis paralysis) - better safe than sorry.

andralex · 2017-06-09T18:09:40Z

@CyberShadow could you please suggest a step forward for https://issues.dlang.org/show_bug.cgi?id=16615? Thanks.

CyberShadow · 2017-06-09T18:13:54Z

WONTFIX. Although it might not be obvious, killing a process is inherently a very platform specific operation. There is and cannot be an obviously correct way to do it on both platforms (Windows and POSIX), because the two platforms manage processes in different way. Windows does not have signals; POSIX does not allow specifying exit codes; and reparenting is done differently on both. Furthermore, the example use case presented in the issue (killing a process tree) is not possible to correctly implement with just kill - you must suspend the entire process group, and only then kill them, and I'm not sure that even that is sufficient. I would advise against attempting to do it in general, and instead run the process group in a container and kill it wholesale.

andralex · 2017-06-09T18:14:36Z

@CyberShadow OK, please put WONTFIX on it with explanation.

CyberShadow · 2017-06-09T18:18:18Z

Done

quickfur · 2017-06-09T18:32:30Z

IMO, it's OK to add a function to return the OS pid from a Pid object in Phobos. However, going the other way is a bad idea, for the reasons discussed above.

Writing a task manager in D implies knowledge of OS-specific implementation details, and due to differences in Windows vs. Posix, I don't think there is any platform-agnostic way to do this. I.e., I don't think this should be used as a reason for allowing arbitrary conversion of OS pids to std.process Pid objects.

andralex · 2017-06-09T18:33:05Z

Thanks to all!

schveiguy · 2017-06-09T19:32:01Z

kill(int pid, int signalOrExitCode)

I was thinking more along the lines of killProcess(int pid), which just does the same thing kill pid does in Unix, and closing the process does in Windows. Trying to handle all the nuances of the OS options, as you say, isn't possible in an OS-agnostic fashion. In that case we can just defer to the exposed C functions instead.

Even that simple wrapper may be more fluff than substance, as the C functions are pretty straightforward.

CyberShadow · 2017-06-09T19:48:00Z

which just does the same thing kill pid does in Unix

Yeah but kill sends SIGTERM by default, which might not be what the user intends. Windows does not have an equivalent of SIGTERM.

and closing the process does in Windows

You mean from Task Manager?

quickfur · 2017-06-09T22:50:24Z

Haha, @CyberShadow has a good point, kill sends SIGTERM by default, but processes may ignore it and/or do something else if they've installed their own signal handler. I'm not sure there's any common process-terminating operation that has common semantics across Posix and Windows.

schveiguy · 2017-06-10T12:29:01Z

I mean do the thing that people do on that os. Yes sigterm on unix, task manager closing (or force closing) on windows. Maybe a bool flag for force closing which switches to sigkill and force close.

CyberShadow · 2017-06-10T15:06:23Z

How do you propose to implement non-force-closing on Windows?

schveiguy · 2017-06-10T17:19:16Z

Not sure. I searched online for a mechanism but can't find it.

JackStouffer added the Review:Needs Review label Feb 4, 2017

DmitryOlshansky reviewed Feb 8, 2017

View reviewed changes

edi33416 force-pushed the process_enhancement branch 6 times, most recently from 1486096 to 91ce00d Compare April 6, 2017 09:32

edi33416 force-pushed the process_enhancement branch from 91ce00d to 560e2c8 Compare April 10, 2017 11:02

edi33416 changed the title ~~Fix Issue 16615 - convert os pid to std.process Pid~~ Partial Fix Issue 16615 - convert os pid to std.process Pid Jun 6, 2017

edi33416 force-pushed the process_enhancement branch from 560e2c8 to 0ee3059 Compare June 6, 2017 18:27

andralex approved these changes Jun 7, 2017

View reviewed changes

andralex added Review:Needs Work and removed Review:Needs Review labels Jun 7, 2017

edi33416 force-pushed the process_enhancement branch from 0ee3059 to 44fc785 Compare June 7, 2017 22:19

Partial Fix For Issue 16615 - convert os pid to std.process Pid

71ca4c5

edi33416 force-pushed the process_enhancement branch from 44fc785 to 71ca4c5 Compare June 7, 2017 22:27

andralex approved these changes Jun 7, 2017

View reviewed changes

andralex added Merge:auto-merge and removed Review:Needs Work labels Jun 7, 2017

dlang-bot merged commit 15a5a48 into dlang:master Jun 8, 2017

CyberShadow reviewed Jun 8, 2017

View reviewed changes

CyberShadow mentioned this pull request Jun 8, 2017

Fix issue 17479: public ctors for std.process.Pid #5450

Closed

CyberShadow mentioned this pull request Jun 9, 2017

Revert "Partial Fix Issue 16615 - convert os pid to std.process Pid" #5456

Merged

FreeSlave mentioned this pull request Jun 14, 2017

Add spawnProcessDetached #5483

Merged

edi33416 deleted the process_enhancement branch October 9, 2017 13:48

Uh oh!

Conversation

edi33416 commented Feb 3, 2017

Uh oh!

dlang-bot commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackStouffer commented Feb 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyberShadow commented Jun 8, 2017

Uh oh!

CyberShadow commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edi33416 commented Jun 8, 2017

Uh oh!

edi33416 commented Jun 9, 2017

Uh oh!

CyberShadow commented Jun 9, 2017

Uh oh!

andralex commented Jun 9, 2017

Uh oh!

CyberShadow commented Jun 9, 2017

Uh oh!

andralex commented Jun 9, 2017

Uh oh!

CyberShadow commented Jun 9, 2017

Uh oh!

dlang-bot commented Feb 3, 2017 •

edited

Loading

CyberShadow commented Jun 8, 2017 •

edited

Loading

schveiguy commented Jun 9, 2017 •

edited

Loading

CyberShadow commented Jun 9, 2017 •

edited

Loading