Re: [PATCH] tee: Add --pipe-check to allow instantly detecting closed outputs

[Carl Edquist]

On Fri, 2 Dec 2022, Arsen Arsenović wrote:

> I'm concerned with adding such a behavior change by default still.  I 
> can imagine this "lifetime extension" properly having been relied on in 
> the last many decades it has been around for ;)

That's fair! :)  I'd be curious to hear about a use-case for that; but 
anyway yeah if it's an option at least there won't be any surprises.


> > Although tee has multiple outputs, you only need to monitor a single 
> > output fd at a time.  Because the only case you actually need to catch 
> > is when the final valid output becomes a broken pipe.  (So I don't 
> > think it's necessary to poll(2) all the output fds together.)
>
> That is technically true, but I think coupling this to two FDs might 
> prove a bit inelegant in implementation (since you'd have to decide 
> which entry from an unorganized list with holes do you pick?  any of 
> them could spontaneously go away), so I'm not sure the implementation 
> would be cleaner that way.

It'd be best to explain with a patch - I'll plan to send one later for a 
concrete example.

But to try to answer your question:

> you'd have to decide which entry from an unorganized list with holes do 
> you pick?

So as you've seen there is a "descriptors" array corresponding to all the 
outputs.  What I had in mind is to maintain an int that keeps track of the 
index of the first item in the descriptors array that is still valid. 
(They are actually FILE *'s, which are set to NULL when they become 
invalid.)

So, you'd start with the first one (which happens always to be stdout). 
If the output at the current index (starting with stdout) ever becomes 
invalid (due to broken pipe detection, or due to other non-fatal write 
failure), then increase the index until the next valid output is found 
(skipping NULL entries).  If the last output is closed, it's not really 
important what happens to the index - it can be left as-is or set to -1; 
it won't be used again in any case.

> any of them could spontaneously go away

I think it should be OK just to check the current output index in the list 
and advance if that one closes.  If a pipe becomes broken in the middle of 
the list, I think it's fine to let it be.

Why is this OK?

First of all, the current index still refers to a valid output.  That 
means you are _not_ in a situation where all outputs are broken pipes, so 
the right thing to do is to continue waiting for input.

Then, if input arrives, it will get written to all the valid (ie, 
non-NULL) outputs, including the one that has now become a broken pipe 
(without being detected).

But this is OK, because (as we've discussed before) we should already be 
ignoring SIGPIPE (and handling EPIPE), to prevent races that can happen 
where a fatal SIGPIPE comes in after a read() and before the corresponding 
write().  So, this broken pipe gets removed due to the write failure 
(EPIPE), rather than the broken-pipe detection.

But it does not affect the lifetime of the tee process, as any time poll() 
is waiting, the index will point to an output that is still valid and is 
not (yet) a broken pipe.

...


But the proof is in the pudding; so I'll try to draft something up and you 
can see what you think technically & aesthetically...


Cheers!
Carl