Re: tee: not checking for EAGAIN or EWOULDBLOCK causing missing output to terminal

[Kamil Dudka]

I see two major issues with the patch:

1. If the file descriptor operates in non-blocking mode and it starts
to return EAGAIN, tee will busy-loop (consume 100% CPU) until the file  
descriptor becomes ready again.

2. If fwrite() fails with EAGAIN, you cannot repeat the call with the same 
arguments because something might have been already written before EAGAIN was 
returned from a syscall.  In that case, you would try to write the already 
written data repeatedly, which is not desired.

A few months ago I proposed a patch that was free of the above issues but it 
addressed EAGAIN on tee's output only:

    https://lists.gnu.org/archive/html/coreutils/2018-09/msg00010.html

The bad news is that the patch was rejected because a workaround exists.

Kamil


On Wednesday, July 24, 2019 6:39:02 PM CEST Austen W Lauria wrote:
> Hi all,
> 
> I see that this issue was reported prior in the mailing list, but just
> wanted to bring it back to attention.
> 
> Customers have seen problems running our apps that produce a ton of output
> (such mpirun that just dumps standard out) when using tee. The file gets
> completely written to, but the output to the terminal is cut short. And
> this appears to me to be an issue
> where tee is not checking for EAGAIN in the fwrite() and read() call.
> 
> For example:
> 
> $. mpirun -np 5 ./stdout.exe 1000000 2>&1
> 
> | /smpi_dev/awlauria/coreutils/usr/bin/tee sten_out
> 
> Will produce about about 5 million lines of stdout (per 1 million per
> process). Intermittently, not all output will be returned to the users
> terminal. And when mpirun exits first in this case, the user will also be
> greeted with a "tee: write error" message - probably because mpirun closed
> the stdout/stderr on exit while tee was still running. If I modify mpirun
> to just hang and not exit, tee will hang to infinity in the tee_files()
> call (can see using pstack).
> 
> Using strace, I was able to narrow the failure to an EAGAIN on fwrite() in
> tee:
> 
> write(1, "\nPrint to stdout line 13721 a mu"..., 1290) = -1 EAGAIN
> (Resource temporarily unavailable) <--- The user program output.
> write(2, "tee: ", 5)                    = -1 EAGAIN (Resource temporarily
> unavailable)
> write(2, "standard output", 15)         = -1 EAGAIN (Resource temporarily
> unavailable)
> 
> I have a proposed patch here that solves the issue for us - I verified on
> coreutils master. Can you take a look and provide some feedback? Thanks!
> 
> diff --git a/src/tee.c b/src/tee.c
> index d3aecc7..13b4baf 100644
> --- a/src/tee.c
> +++ b/src/tee.c
> @@ -229,19 +229,30 @@ tee_files (int nfiles, char **files)
>    while (n_outputs)
>      {
>        bytes_read = read (STDIN_FILENO, buffer, sizeof buffer);
> -      if (bytes_read < 0 && errno == EINTR)
> +      if (bytes_read < 0
> +          && (errno == EINTR || errno == EAGAIN || errno == EWOULDBLOCK))
> {
>          continue;
> +      }
> +
>        if (bytes_read <= 0)
>          break;
> 
>        /* Write to all NFILES + 1 descriptors.
>           Standard output is the first one.  */
> -      for (i = 0; i <= nfiles; i++)
> +      i = 0;
> +      while (i <= nfiles) {
>          if (descriptors[i]
>              && fwrite (buffer, bytes_read, 1, descriptors[i]) != 1)
>            {
>              int w_errno = errno;
> -            bool fail = errno != EPIPE || (output_error ==
> output_error_exit
> +            if(w_errno == EAGAIN || w_errno == EINTR || w_errno ==
> EWOULDBLOCK) {
> +              if(descriptors[i] == stdout) {
> +                clearerr (stdout); /* Avoid redundant close_stdout
> diagnostic.  */
> +              }
> +              continue;
> +            }
> +
> +            bool fail = (errno != EPIPE)|| (output_error ==
> output_error_exit
> 
>                                            || output_error ==
> 
> output_error_warn);
>              if (descriptors[i] == stdout)
>                clearerr (stdout); /* Avoid redundant close_stdout
> diagnostic.  */
> @@ -256,6 +267,8 @@ tee_files (int nfiles, char **files)
>                ok = false;
>              n_outputs--;
>            }
> +          i++;
> +      }
>      }
> 
>    if (bytes_read == -1)