Unbuffered I/O breaks some valid usages

[nertpinx]

fish version: 3.2.0
uname: Linux cjohnson 5.11.6-gentoo-x86_64 #1 SMP Thu Mar 18 10:26:07 CET 2021 x86_64 Intel(R) Core(TM) i7-8650U CPU @ 1.90GHz GenuineIntel GNU/Linux
TERM: xterm-kitty

Builtin echo function (and probably other built-ins as well since 616d301) are using unbuffered writes to stdout even when the output is redirected to a file. This causes issue when writing to kernel interface files where each write is consumed separately.

Example of the behaviour:

# strace -P /sys/class/power_supply/BAT0/charge_control_end_threshold -e write --signal !SIGCHLD fish -c 'builtin echo 100 >/sys/class/power_supply/BAT0/charge_control_end_threshold'
strace: Requested path "/sys/class/power_supply/BAT0/charge_control_end_threshold" resolved into "/sys/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:19/PNP0C09:00/PNP0C0A:00/power_supply/BAT0/charge_control_end_threshold"
write(4, "1", 1)                        = 1
write(4, "0", 1)                        = -1 EINVAL (Invalid argument)
write: Invalid argument
+++ exited with 0 +++
# cat /sys/class/power_supply/BAT0/charge_control_end_threshold
1

Yes, the other issue is that the command does not fail and ends with 0. Let me know if I need to file another issue for that. In order to make this work (and charge my battery to a percentage higher than one digit numbers) it is enough to use /bin/echo, bash, echo 100 | cat >/... or basically any other solution. The issue is not that this would not be possible to do otherwise, but it is completely unclear for the user to see what is happening.

The change to unbuffered I/O was done because of #3748. Even though I could argue that it makes the shell behave unexpectedly and that issue could've been fixed differently, I think it would be enough if the writes were buffered when being redirected as that could, probably, deal with most issue possibly arising from this.

[ridiculousfish]

Heh, are we really issuing one write per character? That's pretty silly...certainly we can fix that.

[faho]

Hahaha, yeah, running streams.out.push_back() writes to the fd immediately, and echo does that for each character.

I'm not entirely sure what the general buffering strategy here should be - chunking seems faster, but can lead to weird results. However, echo in particular should just write in one go. That's conceptually what it does, and it's easy to change.

Given that this was changed in 2.6, four years ago, I'm removing the "regression" label. It's not, in a practical sense, something that's changed recently.

[faho]

Alright, I've changed this for echo, and that's really the biggest offender. All other builtins have some form of chunking - e.g. string will append each argument separately, which is quite natural.

They will, however, append a newline or space separately with something like streams.out.push_back(L'\n').

Also printf will append some escapes immediately.

The best chunking strategy I can come up with here is something like "after newline or every 512 bytes" (or some other power-of-2).

[nertpinx]

@faho The pity is that you need to deal with this in the code when the system is perfectly capable of doing so itself. I'm guessing that even getting a filebuf is not possible in the scenario, right?

[faho]

After further deliberation, I don't think there is anything else to do here, since now all builtins do some form of sensible-ish chunking, and it doesn't seem like something that often comes up.