Bug 17250 – ProcessPipes (std.process) should provide a test for a null pid

The ProcessPipes struct provides a method, pid(), that returns the Pid of the attached process. The pid() method asserts if the private member is null: @property Pid pid() @safe nothrow { assert(_pid !is null); return _pid; } The problem is that there is no method available to determine if pid is null prior to call the pid() method. In idiomatic use this unlikely to be an issue, as entering a block requiring access to the pid is typically conditioned on successful creation of the pid. An example from the doc: auto pipes = pipeProcess("my_application", Redirect.stdout | Redirect.stderr); scope(exit) wait(pipes.pid); ... more code ... In the above, the scope exit block is only entered if pid is successfully created. However, if process creation is deferred, the pid might never be assigned. E.g. ProcessPipes pipes; scope(exit) wait(pipes.pid); ... code ... pipes = pipeProcess("my_application", Redirect.stdout | Redirect.stderr); ... code ... The above will fail in the 'wait(pipes.pid)' call if the _pid member is null. What seems desired is a way to write something equivalent to: scope(exit) if (!pipes.isNullPid) wait(pipes.pid)

I think the solution here is to remove the assert and expect the user to check if the result of pid is null.

PR: https://github.com/dlang/phobos/pull/5621

(In reply to Jon Degenhardt from comment #1) > The problem is that there is no method available to determine if pid is null > prior to call the pid() method. Why not: if (pipes !is ProcessPipes.init) (In reply to RazvanN from comment #2) > I think the solution here is to remove the assert and expect the user to > check if the result of pid is null. I think that's OK too, though it has the small downside that null dereferencing bugs are harder to diagnose on non-Windows systems. We could also add an implicit conversion to bool.

(In reply to Vladimir Panteleev from comment #4) > (In reply to Jon Degenhardt from comment #1) > > The problem is that there is no method available to determine if pid is null > > prior to call the pid() method. > > Why not: if (pipes !is ProcessPipes.init) Isn't 'is' a compile-time expression? The test needs to be run-time. Though that would mean 'if (pipes != ProcessPipes.init)' would work. Either way, these seem like very obscure ways to do such a test. > > (In reply to RazvanN from comment #2) > > I think the solution here is to remove the assert and expect the user to > > check if the result of pid is null. > > I think that's OK too, though it has the small downside that null > dereferencing bugs are harder to diagnose on non-Windows systems. > > We could also add an implicit conversion to bool. Implicit conversion to bool sounds pretty good. Then it'd become: scope(exit) if (pipes) wait(pipes.pid) For me the downside of simply removing the assert and returning a null Pid object is that it uses an internal implementation detail of ProcessPipes to describe the state of the ProcessPipes instance. Modularity is better served by separating the ProcessPipes state from the state internal _pid member, at least in the ProcessPipes API. (This argument also says the original suggested API name of 'isNullPid' is not a good name.)

(In reply to Jon Degenhardt from comment #5) > (In reply to Vladimir Panteleev from comment #4) > > (In reply to Jon Degenhardt from comment #1) > > > The problem is that there is no method available to determine if pid is null > > > prior to call the pid() method. > > > > Why not: if (pipes !is ProcessPipes.init) > > Isn't 'is' a compile-time expression? The test needs to be run-time. Though > that would mean 'if (pipes != ProcessPipes.init)' would work. Either way, > these seem like very obscure ways to do such a test. Never mind the 'compile-time expression' part, 'is' is a run-time test. Still, it seems very obscure, and it's not obvious that a ProcessPipes struct that has a null PID due to process startup failure will be in the same state as the struct at .init time.

Commits pushed to master at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/b4284db1f937e90fec6ffe4a9e4e2bf4783799bb Fix Issue 17250 - ProcessPipes (std.process) should provide a test for a null pid https://github.com/dlang/phobos/commit/1e296c11d8f8083402f8611d495cdcd015ff6fb3 Merge pull request #5621 from RazvanN7/Issue_17250 Fix Issue 17250 - ProcessPipes (std.process) should provide a test for a null pid merged-on-behalf-of: Vladimir Panteleev <[email protected]>

Commits pushed to stable at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/b4284db1f937e90fec6ffe4a9e4e2bf4783799bb Fix Issue 17250 - ProcessPipes (std.process) should provide a test for a null pid https://github.com/dlang/phobos/commit/1e296c11d8f8083402f8611d495cdcd015ff6fb3 Merge pull request #5621 from RazvanN7/Issue_17250

Commits pushed to dmd-cxx at https://github.com/dlang/phobos https://github.com/dlang/phobos/commit/b4284db1f937e90fec6ffe4a9e4e2bf4783799bb Fix Issue 17250 - ProcessPipes (std.process) should provide a test for a null pid https://github.com/dlang/phobos/commit/1e296c11d8f8083402f8611d495cdcd015ff6fb3 Merge pull request #5621 from RazvanN7/Issue_17250