Re: wait code is fishy

On 8/11/25 14:57, Harald van Dijk wrote:
> On 11/08/2025 08:52, Denys Vlasenko wrote:
>> On 8/11/25 03:21, Harald van Dijk wrote:
>>> On 10/08/2025 22:30, Denys Vlasenko wrote:
>>>> The reason for "pid == 0" retrying NONBLOCK wait for more processes
>>>> is unclear: if we got here, we already did wait for processes
>>>> just now, got none, waited for signals and got a not-SIGCHLD one,
>>>> so there should not be more processes to wait for.
>>>> A comment explaining it would be nice?
>>>
>>> A signal that gets sent to a process group can simultaneously be> a nonfatal signal to the shell, and a fatal signal to the child,> where the nonfatal signal to the shell may get processed by
>>> the shell before the SIGCHLD resulting from the fatal signal> to the child. I do think it's possible for that extra> loop iteration to find more processes that have terminated.
>>>
>>> Whether this is an important case to handle, I do not know.
>>
>> Of course any additional iteration of wait3(WNOHANG) has a chance
>> of finding more processes exited or stopped.
>>
>> But by this logic, we may as well decide to do seventeen
>> iterations of it, not two.
> 
> I think this is misrepresenting what I was saying, and what the code is doing.
> 
> Currently, the code loops until it gets a result telling it there are no more processes that finished. You're suggesting looping as long as it gets a result telling it there *are* processes that finished. The difference is in the case where the shell got no information at all on whether a process finished, because the shell received a signal.

         rpid = 1;
         do {
                 pid = waitone(block, jp);
                 rpid &= !!pid;    <<<<<<<<<<<<<<<<<<<<<<<<< HERE
                 block &= ~DOWAIT_WAITCMD_ALL;
                 if (!pid || (jp && jp->state != JOBRUNNING))
                         block = DOWAIT_NONBLOCK;
         } while (pid >= 0);
         return rpid;


If we got pid == 0, it happens in exactly one case:
we did waitpid(WNOHANG), got 0 ("children exist but none yet
changed its state"), then waited for a signal and got one,
which wasn't SIGCHLD.

The above behavior is requested by only one caller: waitcmd()
- the "wait" builtin.

"wait" builtin is required to be interruptible by signals and
to set exitcode to (128 + sig).

Therefore, if we got pid == 0, we don't need to wait
for any more processes. We need to return to waitcmd()
and let it set $? accordingly.

Instead, we do yet another loop.


> You're suggesting (please correct me if I am misrepresenting
> what you meant) that because the shell received a signal,
> we already know that there are no more processes that finished,

The last part is incorrect. I'm not saying that we know
that there are no more processes that finished.
I'm saying we don't care at this point: we know that we are "wait"
builtin and we got interrupted by a signal.

> For processes that finish before the signal, and processes that> finish after the signal, I agree.
> But those aren't the only situations: I'm pointing out that
> it's not only possible, but reasonable and common, that
> a process finished and the shell received a signal at exactly
> the same time. That is the situation that the code currently
> handles, that your proposed change would not handle.

Yes, current code, after getting a signal, will try and see
whether there are processes with changed state. I understand that.

I'm saying that it is not necessary. We already got the signal.

But actually, my bigger point is that the code is awfully convoluted.

It's ok to leave the logic unchanged, but perhaps it should be
tweaked to be more understandable. Unreadable code snippet example:

                 if (err || (err = -!block))
                         break;

Assignment in the conditional, just what we need to be confused.

Or look at this:

/* mode flags for dowait */
#define DOWAIT_NONBLOCK 0
#define DOWAIT_BLOCK 1
#define DOWAIT_WAITCMD 2
#define DOWAIT_WAITCMD_ALL 4

First two modes tell dowait() what to do:
do a nonblocking, or blocking wait3().

But DOWAIT_WAITCMD name uses a different convention.
Instead of telling dowait() what to do, it tells dowait()
*where the call came from*! So, what would dowait() do?
Code reader has no idea...
Why not name it, for example, DOWAIT_CHILD_OR_SIGNAL instead,
so that it describes what dowait() will wait for?

Currently these three small functions are in such shape that one needs
at least ~30 minutes to grasp what's going on.
I, in fact, needed several test runs with instrumentation to understand.