[Toybox] [PATCH] xputs: Do flush

[Yi-Yo Chiang]

Thanks! Adding TOYFLAG_NOBUF worked.

I feel the same way about "manual flushing of the output buffer is a
terrible interface". I asked myself the question "Why am I manually
flushing so much? There must be a better way..." multiple times when I
wrote the other patch that does s/xprintf/dprintf/, s/xputs/xputsn/


On Mon, May 20, 2024 at 8:36 PM enh <enh at google.com> wrote:

> On Sun, May 19, 2024 at 10:56 PM Rob Landley <rob at landley.net> wrote:
> >
> > On 5/18/24 21:53, Yi-Yo Chiang wrote:
> > > What I wanted to address with this patch are:
> > >
> > > 1. Fix this line of
> > > xputs()
> https://github.com/landley/toybox/blob/master/toys/net/microcom.c#L113
> > > The prompt text is not flushed immediately, so it is not shown to the
> user until
> > > the escape char is entered (which defeats the purpose of the prompt,
> that is to
> >
> > I agree you've identified two problems (unflushed prompt, comment not
> matching
> > code) that both need to be fixed. I'm just unhappy with the solutions,
> and am
> > concerned about a larger design problem.
> >
> > I implemented TOYFLAG_NOBUF and applied it to this command. The result
> compiles
> > but I'm not near serial hardware at the moment, does it fix the problem
> for you?
> >
> > Trying to fix it via micromanagement (adding more flushing and switching
> some
> > but not all output contexts in the same command between FILE * and file
> > descriptor) makes my head hurt...
> >
> > Adding flushing to xputs() is an Elliott question is because he's the
> one who
> > can presumably keep stdio buffer semantics in his head. They make zero
> sense to
> > me. I added a flush to xputsn() because in line buffering mode it was the
> > "dangerous" one that had no newline thus no flush, but then when we go
> to block
> > buffering mode xputs() needs a flush just like xputsn() would, and MAYBE
> it's
> > good to have the flush because in line buffer mode it would be a NOP?
> Except the
> > command selected block buffering mode precisely BECAUSE it didn't want
> to flush
> > each line, so why should xputs() do it when the command asked not to?
> And if
> > xputs() is doing it, why is xprintf() _not_ doing it? And if xprintf()
> _is_
> > doing it, then we're back to basically not buffering the output...
>
> this to me was exactly why it should be "everything flushes" or
> "nothing flushes". not "some subset of calls for some subset of
> inputs", because no-one can keep all the special cases in their heads.
> and "everything flushes" is problematic from a performance
> perspective, so "nothing flushes" wins by default. (but, yes, when we
> have our own kernel, have a time-based component to buffering layer's
> flushing is an interesting idea :-) )
>
> > > tell the user what the escape char is) and stdout is flushed by
> handle_esc.
> > > To fix this we either make xputs() flush automatically, or we just add
> a single
> > > line of manual flush() after xputs() in microcom.c.
> > > Either is fine with me.
> >
> > When I searched for the first xputs in microcom I got:
> >
> >   xputsn("\r\n[b]reak, [p]aste file, [q]uit: ");
> >   if (read(0, &input, 1)<1 || input == CTRL('D') || input == 'q') {
> >
> > Which is a separate function (the n version is no newline, it does not
> add the
> > newline the way libc puts() traditionally does), with its own flushing
> > semantics: xputsn() doesn't call xputs(), and neither calls or is called
> by
> > xprintf(). "Some functions flush, some functions don't" is a bit of a
> design
> > sharp edge.
>
> (yeah, exactly.)
>
> > The larger problem is manual flushing of the output buffer is a terrible
> > interface, and leads to missing error checking without which a command
> won't
> > reliably exit when its output terminal closes because the whole SIGPIPE
> thing
> > was its own can of worms that even bionic used to manually mess with.
> Which is
> > why I originally made toybox not ever do that (systemic fix) but I got
> > complaints about performance.
> >
> > Your other patch changes a bunch of xprintf() to dprintf() which is even
> _more_
> > fun because dprintf() writes directly to the file descriptor (bypassing
> the
> > buffer in the libc global FILE * instance "stdio"), which means in the
> absence
> > of manual flushing the dprintf() data will go out first and then the
> stdio data
> > will go out sometime later, in the wrong order. Mixing the two output
> formats
> > tends to invert the order that way, unless you disable output buffering.
>


Which is the reason I replaced those all with the "flush" functions
(xputsn) or direct fd-write functions (dprintf), so that their order won't
shuffle.
Anyway the problem is moot now that we have TOYFLAG_NOBUF.


> >
> > I like systematic solutions that mean the problem never comes up again.
> Elliott
> > has been advocating the whack-a-mole "fix them as we find them" approach
> here
> > which I am not comfortable with. I've been leaning towards adding a
> > TOYFLAG_NOBUF so we can select a third buffering type, and go back to
> "do not
> > buffer output at all, flush after every ANSI write function" for at
> least some
> > commands I'd like to be able to reason about. Especially for commands
> like this
> > where output performance really doesn't seem like it should be an issue.
>
> +1 --- an inherently interactive program like this seems reasonable to
> be unbuffered. (except for file transfer?)
>
> > And OTHER implementations can't consistently get this right, which is
> why whether:
> >
> >   for i in {1..100}; do echo -n .; sleep .1; done | less
> >
> > Produces any output before 10 seconds have elapsed is potluck, and
> varies from
> > release to release of the same distro.
> >
> > Oh, and the gnu/crazies just came up with a fourth category of write as a
> > gnu/extension: flush after NUL byte.
> >
> > https://lists.gnu.org/archive/html/coreutils/2024-03/msg00016.html
>
> (fwiw, i think that was just some internet rando asking for it, no?
> and they didn't actually implement it?)
>
> > It's very gnu to fix "this is too complicated to be reliable" by adding
> MORE
> > complication. Note how the problem WE hit here was 1) we didn't ask for
> LINEBUF
> > mode, 2) \r doesn't count as a line for buffer flushing purposes anyway,
> 3) the
> > new feature making it trigger on NUL instead _still_ wouldn't make \r
> count as a
> > line for buffer flushing purposes.
> >
> > My suggestion for a "proper fix" to the problem _category_ of small
> writes being
> > too expensive was to have either libc or the kernel use nagle's
> algorithm for
> > writes from userspace, like it does for network connections. (There was
> a fix to
> > this category of issue decades ago, it just never got applied HERE.)
> >
> > But that hasn't been popular, and it's a pain to implement in userspace
> (because
> > we don't have access to mulitple cheap timers like the kernel does, we
> need to
> > take a signal and there's both a limited number of signals).
>
> do you run on anything that doesn't have real-time signals? i was
> going to say that -- since toybox is a closed world -- you could just
> use SIGUSR2, but i see that dhcp is already using that! but if you can
> assume real-time signals, there are plenty of them...
>
> > So that's not what
> > the ANSI committee decided to do in 1988, instead overlaying terrible
> semantics
> > on Ken/Dennis/Doug's elegant original design.
> >
> > Now that the kernel has vdso, this seems to me like an obvious use for
> it: have
> > a vdso write() function detect fd 1 and write the bytes(s) into a page
> size ring
> > buffer with incremented output position indicator, maybe with the buffer
> taking
> > a soft fault on first write to enable the kernel timer to flush it 1/10
> second
> > later (whatever the existing nagle timeout is), and big writes after
> small
> > writes could even become an iovec or something (maybe using cmpxchg to
> rewind
> > the buffer position indicator if the timer expiration hasn't advanced it
> yet) to
> > avoid extra syscalls when flushing the to buffer full instead of timer
> expiration.
> >
> > Then userspace does the obvious thing and the kernel makes it fast. As
> > Ken/Dennis/Doug intended.
> >
> > > 2. The comment string before xputs() is misleading and I wanted to fix
> that.
> > > I was misled by the comment line, and incorrectly thought xputs() was
> flushing.
> >
> > Yeah, my bad. It's changed multiple times because of arguments about
> > performance. It's xputsn() and xputsl() that are currently flushing.
> (Which made
> > sense when "line buffer vs no buffer" were the options, vs block buffer
> vs line
> > buffer and you couldn't ask for NOT buffered. Commit 3e0e8c687eee back
> in January.)
> >
> > Oh, and I've had similar arguments on the READ FILE * side, wanting
> getline() to
> > pass along the leftover readahead data after \n to child processes, ala
> "read i;
> > zcat > $i" which has the problem of getline() reading a block of data
> into the
> > FILE * buffer which then isn't available to zcat.
> >
> > I eventually (https://landley.net/notes-2023.html#24-06-2023) worked
> out that on
> > pipes you can use O_DIRECT to prevent collating and thus most instances
> of
> > readahead, and on seekable files you can fseeko() on the file pointer to
> force
> > it to back up to where it THINKS it is and thus make the readahead data
> > available through the filehandle again, and that SHOULD cover the
> majority of
> > cases the shell cares about?
> >
> > But that was AFTER I complained at the posix guys that there's no
> portable way
> > to ask an input FILE * how much data is in its buffer and wound up with
> SIX
> > different contexts (two standards committees, three linux library
> maintainers,
> > plus BSD) pointing fingers at each other, which I ranted about in my
> blog:
> >
> > https://landley.net/notes-2022.html#19-04-2022
> >
> > Thus the old saying that there are two hard problems in computer science:
> > naming things, cache invalidation, and off by one errors.
> >
> > > I only discovered otherwise after iterations of trial-and-error.
> > > To fix this is to either make xputs() flush automatically, or just
> update the
> > > comment string to clarify that xputs() does _not_ flush.
> > > Again either path is fine with me. I just merged the fixes of the two
> problems
> > > into one because they share the former solution. Plus xputsl() and
> xputsn() are
> > > also flushing, so I thought / believed xputs() was mistakenly made
> non-flushing.
> >
> > Even xprintf() _used_ to flush. All the xout() functions did. And then
> grep was
> > too slow... (See "historical performance complaints", above.)
> >
> > > Since the intention _is_ to keep xputs() non-flushing, how about we
> just update
> > > the comment of xputs() (2), and add the line of flush in microcom.c
> (1)?
> >
> > As I said, I did TOYFLAG_NOBUF and hope it works for you?
> >
> > I don't know if xputs() should have the flushing added or the comment
> removed,
> > that's a design question for Elliott.
>
> comment removed makes sense to me?
>
> i like the x<foo>() => "it's <foo>, but exits on error" pattern. super
> consistent and easy to reason about, and easy to read when you know
> it, and easy to fix code that should/shouldn't error in either
> direction, by adding or removing the 'x'. (one of the biggest problems
> with the old xflush() was that it mostly _didn't_ flush! xferror() is
> a weird name, but at least it does what it says on the tin :-) )
>
> > Rob
> > _______________________________________________
> > Toybox mailing list
> > Toybox at lists.landley.net
> > http://lists.landley.net/listinfo.cgi/toybox-landley.net
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.landley.net/pipermail/toybox-landley.net/attachments/20240520/81eaf99e/attachment.htm>