[Toybox] [PATCH] watch: flush the buffer each round.

[enh]

On Tue, Mar 12, 2024 at 8:45 AM Rob Landley <rob at landley.net> wrote:
>
> Hello from Minneapolis: https://mstdn.jp/@landley/112078501045637288
>
> Still _really_ fried from my move, but at least chipping away at the email
> backlog...

at least it's the same timezone :-)

> On 3/11/24 12:27, enh wrote:
> > On Fri, Mar 8, 2024 at 5:43 PM Rob Landley <rob at landley.net> wrote:
> >>
> >> I remember when the xprintf() family would do a flush and check for errors each
> >> write. That's why code like:
> >>
> >>         dprintf(1, "%c", pad<cmdlen ? '*' : ' ');
> >>         if (width) xputs(ss+(width>ctimelen ? 0 : width-1));
> >>         if (yy>=3) dprintf(1, "\r\n");
> >>
> >> Was allowable.
> >>
> >> I also remember when we had an xflush() that would catch errors if stdout
> >> barfed, instead of calling fflush() and ignoring the return code.
> >
> > yeah, i didn't understand why you removed xflush().
>
> The commit comment on 3e0e8c687eee tried to explain: years ago xflush() got
> broken to have a "don't actually flush" argument, and thus didn't do what its
> name said anymore, so I renamed it to what it did (check errors and nothing
> more) and added a manual flush in the places that was passing in the "actual
> flush" argument. (So that change shouldn't have made it worse.)
>
> My original approach to FILE * was to just always flush (ala xprintf) but there
> were objections to that, which I said would open a can of micromanagement worms.
> Then I changed the default buffer type to be unbuffered instead, and there were
> objections to that, which I said would open a can of micromanagement worms...

(well, yeah, the "don't actually flush" argument was weird. but
there's nothing inherently wrong with "flush and exit on failure", and
for those places that _do_ have an opinion on when is a good time to
flush, using that rather than directly using fflush would be
reasonable?)

> > but none of this
> > is relevant here --- the problem here is that we're not flushing _at
> > all_, so ToT watch is just broken.
>
> Micromanagement, yes.

not really. there's an obvious point for watch to flush --- "that's
this round of output done, and i'm an interactive program". no
heuristic is going to be right for that because it's going to depend
on stuff like "what's the update frequency? how much attention is the
user paying? what's the content?". `watch ls -l` with a 5s frequency
being a second or so behind is fine. `watch date` with a 1s frequency
not.

> >> This code is mixing dprintf(1, ...) with printf() and fflush(stdout) in a way
> >> that seems unlikely to end well, and I would like to think it through when I'm
> >> not moving house. (Or have somebody whose brain isn't jello reassure me that
> >> it's coherent.)
> >
> > not in the ToT copy it isn't. maybe you have local changes?
>
> Hmmm, yup looks like I was already fixing this command up to not use FILE * at
> all anymore (what is this, my fourth attempt to get away from FILE * buffering
> damage in a systematic way?) and got distracted partway through...

what's the motivation in watch? i don't see any benefit? stdio with
explicit flush is _great_ for this use case, where the caller has a
specific "and i'm done for now" point that _it_ knows.

> > /tmp/toybox$ grep dprintf toys/other/watch.c
> > /tmp/toybox$
> >
> >> Ideally, I would wean it entirely off of the conceptually broken FILE * output
> >> nonsense entirely to dprintf() and write(), because it's intentionally doing
> >> progressive output and trying to micromanage cache management there is unending
> >> pain. (We need a stdout with the nagle algorithm. Why did they only bother to do
> >> that for network sockets?)
> >
> > aye, you keep saying, but that doesn't exist yet, and watch is broken today :-)
>
> The date on the file is February 10th, so I was working on this last month.
>
> Alas, my life's been a bit hectic recently. Trying to shovel out now...
>
> > that said, my previous patch was suboptimal --- there are `continue`s
> > in the loop, so the flush makes more sense at the top. also i noticed
> > by inspection trying to work out what you meant about mixing stdio and
> > direct fd access (i don't use watch myself; this was a reported bug)
> > that -b was broken.
>
> My brain is currently trying to filk the Steven Universe song "other friends" to
> be a demand about test cases and reproduction sequences, but reading the code I
> see the typo.
>
> > so the new patch (attached) is a better fix. but
> > that fixes the normal case and -b for me...
>
> Spraying down the code with manual fflush() calls strikes me as a code smell
> that means "this will break the next time anybody touches anything".

in general, yes. but (as i've argued above) i really think the
opposite for something like watch. and i think that's actually true in
general --- the cases i've had to fix have all basically been these
kinds of interactive programs where there's a specific "done for now!"
point in the code, and we need a flush there.

(at the risk of encouraging you, the way to automate this would
probably be "thou shalt not block other than on i/o without flushing
--- so any sleep or poll or whatever must flush". but that's going to
flush in places where you don't care, involve more "magic" behind the
scenes and -- as i've just argued -- not be solving any real problem.
i'll change my mind if/when we start hitting cases where "spraying
down" is actually true, rather than "insert one flush at the
semantically meaningful point".)

> But I mentioned still being fried from the move, which makes me extra irritable.
> (When did U-haul become a scam? That's new since the last time I used them...)

yeah, probably not the best time to bring up something that's always
wound you up for some reason! (don't blame me --- like i say, i don't
use watch. this was a user :-))

> Rob