https://issues.dlang.org/show_bug.cgi?id=17250

Jon Degenhardt <jrdemail2000-dlang@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|ProcessPipes (std.process)  |ProcessPipes (std.process)
                   |should provide a test for a |should provide a test for a
                   |null pid struct             |null pid

--

https://issues.dlang.org/show_bug.cgi?id=17250

Jon Degenhardt <jrdemail2000-dlang@yahoo.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2

--- Comment #1 from Jon Degenhardt <jrdemail2000-dlang@yahoo.com> ---
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)

--

https://issues.dlang.org/show_bug.cgi?id=17250

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |razvan.nitu1305@gmail.com

--- Comment #2 from RazvanN <razvan.nitu1305@gmail.com> ---
I think the solution here is to remove the assert and expect the user to check if the result of pid is null.

--

https://issues.dlang.org/show_bug.cgi?id=17250

--- Comment #3 from RazvanN <razvan.nitu1305@gmail.com> ---
PR: https://github.com/dlang/phobos/pull/5621

--

https://issues.dlang.org/show_bug.cgi?id=17250

RazvanN <razvan.nitu1305@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |pull

--

https://issues.dlang.org/show_bug.cgi?id=17250

--- Comment #4 from Vladimir Panteleev <dlang-bugzilla@thecybershadow.net> ---
(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.

--

https://issues.dlang.org/show_bug.cgi?id=17250

Vladimir Panteleev <dlang-bugzilla@thecybershadow.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Hardware|x86                         |All
                 OS|Mac OS X                    |All

--

https://issues.dlang.org/show_bug.cgi?id=17250

--- Comment #5 from Jon Degenhardt <jrdemail2000-dlang@yahoo.com> ---
(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.)

--

https://issues.dlang.org/show_bug.cgi?id=17250

--- Comment #6 from Jon Degenhardt <jrdemail2000-dlang@yahoo.com> ---
(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.

--

https://issues.dlang.org/show_bug.cgi?id=17250

--- Comment #7 from github-bugzilla@puremagic.com ---
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 <github@thecybershadow.net>

--