bup 0.33.8 may truncate output when run interactively

[Rob Browning]

For example, it may truncate the output of "bup ls branch" when run from
a terminal. We're working on a fix, and expect to make a 0.33.9 release
with it fairly soon.

Thanks
--
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2011-07-10 E6A9 DA3C C9FD 1FF8 C676 D2C4 C0F0 39E9 ED1B 597A
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

[Rob Browning]

When the control pipe indicates it's time to quit, finish
filtering/forwarding any existing source output first. The truncation
was easy to reproduce with an interactive "bup ls BRANCH" listing a
lot of saves.

This should fix:

39aced66ad1a6438c4ea9463621a2fa18120eb74
main: don't close filter pipes from another thread

Thanks to Christian Wolf for reporting the problem.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---
dev/with-tty | 14 ++++++++++++++
lib/bup/main.py | 29 ++++++++++++++++++++++-------
note/0.33.x.md | 5 +++++
test/ext/test-ls | 16 ++++++++++++++++
4 files changed, 57 insertions(+), 7 deletions(-)
create mode 100755 dev/with-tty

diff --git a/dev/with-tty b/dev/with-tty
new file mode 100755
index 00000000..99be7857
--- /dev/null
+++ b/dev/with-tty
@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+
+set -ueo pipefail
+
+usage() { echo 'Usage: with-tty command [arg ...]'; }
+misuse() { usage 1>&2; exit 2; }
+
+if script -qec true /dev/null; then
+ # linux flavor
+ script -qec "$(printf ' %q' "$@")" /dev/null
+else
+ # bsd flavor
+ script -qe /dev/null "$@"
+fi
diff --git a/lib/bup/main.py b/lib/bup/main.py
index f77de5ac..6801de3b 100755
--- a/lib/bup/main.py
+++ b/lib/bup/main.py
@@ -244,12 +244,31 @@ def filter_output(srcs, dests, control):
read_fds = [control, *srcs]
dest_for = dict(zip(srcs, dests))
pending = {}
+ finish = False
try:
while srcs:
- ready_fds, _, _ = select.select(read_fds, [], [])
+ # When finish is true, transfer any pending data and exit.
+ if not finish:
+ readable_fds, _, _ = select.select(read_fds, [], [])
+ else:
+ # Only reachable with control since only it sets
+ # finish false. Forward any data available (without
+ # blocking) from each src, i.e. whatever's already
+ # pending.
+ if readable_fds == [control]:
+ break
+ readable_fds, _, _ = select.select(read_fds, [], [], 0)
+ # Drop every src for which no data was available.
+ for tried in read_fds:
+ if tried not in readable_fds:
+ read_fds.remove(tried)
+ if tried != control: srcs.remove(tried)
width = tty_width()
- for fd in ready_fds:
- if fd == control:
+ for fd in readable_fds:
+ if control is not None and fd == control:
+ buf = os.read(control, 1)
+ assert buf == b'q'
+ finish = True
continue
buf = os.read(fd, 4096)
dest = dest_for[fd]
@@ -269,10 +288,6 @@ def filter_output(srcs, dests, control):
assert len(split) == 1
if split[0]:
pending.setdefault(fd, []).extend(split)
- if control in ready_fds:
- buf = os.read(control, 1)
- assert buf == b'q'
- break
except BaseException as ex:
pending_ex = ex
# Try to finish each of the streams
diff --git a/note/0.33.x.md b/note/0.33.x.md
index 3963ab8d..fee5c918 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md
@@ -4,6 +4,11 @@ Notable changes in 0.33.x (incomplete)
Bugs
----

+* Internal subcommands (all except `import-rdiff-backup` and
+ `import-rsnapshot`) should no longer truncate their output sometimes
+ when run interactively (e.g. `bup ls ...`) due to a problem
+ introduced in 0.33.8.
+
* When running interactively, `bup` should no longer lose exceptions
raised by the internal "output filter" thread.

diff --git a/test/ext/test-ls b/test/ext/test-ls
index 86d8c58c..174c5af6 100755
--- a/test/ext/test-ls
+++ b/test/ext/test-ls
@@ -17,6 +17,7 @@ else
fi

bup() { "$top/bup" "$@"; }
+with-tty() { "$top/dev/with-tty" "$@"; }

bup-ls() {
if test "$BUP_TEST_REMOTE_REPO"; then
@@ -294,4 +295,19 @@ WVPASSEQ "$(bup-ls -l src/latest/bad-symlink | tr -s ' ' ' ')" \
"$bad_symlink_mode $user/$group $bad_symlink_size $bad_symlink_date src/latest/bad-symlink -> not-there"


+if ! command -v script; then
+ WVSKIP 'Skipping interactive ouput truncation test (no script command)'
+else
+ WVSTART "output isn't truncated when isatty"
+ WVPASS rm -rf src
+ WVPASS mkdir src
+ echo src/some-longer-name-{1..3000} | WVPASS xargs touch
+ WVPASS bup index src
+ WVPASS bup save -n src --strip src
+ WVPASS with-tty "$top/bup" ls -l src/latest | WVPASS wc -l > ls-rows
+ WVPASSEQ 3000 $(<ls-rows)
+fi
+
+
+WVPASS cd "$top"
WVPASS rm -rf "$tmpdir"
--
2.47.2

[Rob Browning]

Previously any external/subprocess commands like import-rdiff-backup
would crash when isatty because filter_output was invoked without the
control argument in run_subproc_command. Make the control argument
optional.

Adjust the import-rsnapshot tests in test-misc to expose the original
problem by running the subcommand with a tty via script when it's
available (which appears likely, since it's a native bsd command, and
in the essential bsdutils package for debian derivatives).

The issue was introduced by

39aced66ad1a6438c4ea9463621a2fa18120eb74
main: don't close filter pipes from another thread

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

lib/bup/main.py | 9 ++++++---
note/0.33.x.md | 4 ++++
test/ext/test-misc | 13 +++++++++++++
3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/lib/bup/main.py b/lib/bup/main.py
index 6801de3b..ba89dbdb 100755
--- a/lib/bup/main.py
+++ b/lib/bup/main.py
@@ -231,7 +231,7 @@ def print_clean_line(dest, content, width, sep=None):
# FIXME: check whether we might need to stash/defer exceptions while
# stderr is diverted.

-def filter_output(srcs, dests, control):
+def filter_output(srcs, dests, control=None):
"""Transfer data from file descriptors in srcs to the
corresponding file descriptors in dests print_clean_line until the
control descriptor produces a 'q' (i.e. quit).
@@ -240,8 +240,11 @@ def filter_output(srcs, dests, control):
assert all(isinstance(x, int) for x in srcs)
assert all(isinstance(x, int) for x in dests)
assert len(srcs) == len(dests)
- assert isinstance(control, int)
- read_fds = [control, *srcs]
+ if control is None:
+ read_fds = list(srcs)
+ else:
+ assert isinstance(control, int), control
+ read_fds = [control, *srcs]

dest_for = dict(zip(srcs, dests))
pending = {}

finish = False
diff --git a/note/0.33.x.md b/note/0.33.x.md
index fee5c918..7035e529 100644
--- a/note/0.33.x.md
+++ b/note/0.33.x.md
@@ -9,6 +9,10 @@ Bugs

when run interactively (e.g. `bup ls ...`) due to a problem

introduced in 0.33.8.

+* External subcommands (i.e. `import-rdiff-backup` and
+ `import-rsnapshot`) should no longer crash when run interactively
+ due to a problem introduced in 0.33.8.

+
* When running interactively, `bup` should no longer lose exceptions
raised by the internal "output filter" thread.

diff --git a/test/ext/test-misc b/test/ext/test-misc
index 60e7b0ca..3c8e08e5 100755
--- a/test/ext/test-misc
+++ b/test/ext/test-misc
@@ -13,6 +13,7 @@ export GIT_DIR="$tmpdir/bup"

bup() { "$top/bup" "$@"; }
sha1sum() { "$top/dev/checksum" -t sha1 "$@"; }
+with-tty() { "$top/dev/with-tty" "$@"; }

WVPASS cd "$tmpdir"

@@ -129,6 +130,9 @@ b"


WVSTART "import-rsnapshot"
+# NOTE: if this test is made optional, say if it eventually depends on
+# rsnapshot being installed, we might want to add other "interactive
+# external command" tests (see script below) to maintain coverage.
export BUP_DIR="$tmpdir/rsnapshot/.bup"
WVPASS rm -rf rsnapshot
WVPASS mkdir rsnapshot
@@ -140,6 +144,15 @@ WVPASS touch rsnapshot/hourly.0/buptest/c/d/e
WVPASS bup import-rsnapshot rsnapshot/
WVPASSEQ "$(bup ls -F buptest/latest/)" "a/
c/"

+if ! command -v script; then

+ WVSKIP 'Skipping interactive import-snapshot test (no script command)'
+else
+ WVPASS rm -rf "$BUP_DIR"
+ WVPASS bup init
+ WVPASS with-tty "$top/bup" import-rsnapshot rsnapshot/
+ WVPASSEQ "$(bup ls -F buptest/latest/)" "a/
+c/"
+fi
WVPASS rm -rf rsnapshot


--
2.47.2

[Rob Browning]

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

test/ext/test-misc | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/test/ext/test-misc b/test/ext/test-misc
index 8a4f6ac5..60e7b0ca 100755
--- a/test/ext/test-misc
+++ b/test/ext/test-misc
@@ -129,19 +129,18 @@ b"


WVSTART "import-rsnapshot"
-D=rsnapshot.tmp
-export BUP_DIR="$tmpdir/$D/.bup"
-WVPASS force-delete $D
-WVPASS mkdir $D
+export BUP_DIR="$tmpdir/rsnapshot/.bup"
+WVPASS rm -rf rsnapshot
+WVPASS mkdir rsnapshot
WVPASS bup init
-WVPASS mkdir -p $D/hourly.0/buptest/a
-WVPASS touch $D/hourly.0/buptest/a/b
-WVPASS mkdir -p $D/hourly.0/buptest/c/d
-WVPASS touch $D/hourly.0/buptest/c/d/e
-WVPASS true
-WVPASS bup import-rsnapshot $D/
+WVPASS mkdir -p rsnapshot/hourly.0/buptest/a
+WVPASS touch rsnapshot/hourly.0/buptest/a/b
+WVPASS mkdir -p rsnapshot/hourly.0/buptest/c/d
+WVPASS touch rsnapshot/hourly.0/buptest/c/d/e
+WVPASS bup import-rsnapshot rsnapshot/

WVPASSEQ "$(bup ls -F buptest/latest/)" "a/
c/"

+WVPASS rm -rf rsnapshot


WVSTART features
--
2.47.2

[Rob Browning]

These patches should address the issue (and some other, related
trouble).

Pushed to 0.33.x.

[Rob Browning]

Since threads just drop exceptions by default, wrap filter_output to
capture any exception it might throw and re-raise it from main.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

lib/bup/main.py | 16 ++++++++++++----
note/0.33.x.md | 13 +++++++++++++
2 files changed, 25 insertions(+), 4 deletions(-)
create mode 100644 note/0.33.x.md

diff --git a/lib/bup/main.py b/lib/bup/main.py
index 8e87f233..6f9c684b 100755
--- a/lib/bup/main.py
+++ b/lib/bup/main.py
@@ -319,15 +319,23 @@ def run_module_cmd(module, args):
if fix_stdout: fix_stream(sys.stdout)
if fix_stderr: fix_stream(sys.stderr)
ctrl_pipe = os.pipe()
- filter_thread = \
- Thread(name='output filter',
- target=lambda : filter_output(srcs, dests, ctrl_pipe[0]))
+ filter_thread_ex = None
+ def run():
+ nonlocal filter_thread_ex
+ try:
+ filter_output(srcs, dests, ctrl_pipe[0])
+ except BaseException as ex:
+ filter_thread_ex = ex
+ filter_thread = Thread(name='output filter', target=run)
filter_thread.start()
try:
result = import_and_run_main(module, args)
+ # main may sys.exit(), so don't put anything that matters here
finally:
os.write(ctrl_pipe[1], b'q')
- filter_thread.join()
+ filter_thread.join()
+ if filter_thread_ex:
+ raise filter_thread_ex
finally:
close_catpipes()
return result
diff --git a/note/0.33.x.md b/note/0.33.x.md
new file mode 100644
index 00000000..3963ab8d
--- /dev/null
+++ b/note/0.33.x.md
@@ -0,0 +1,13 @@
+Notable changes in 0.33.x (incomplete)
+======================================
+
+Bugs
+----
+
+* When running interactively, `bup` should no longer lose exceptions
+ raised by the internal "output filter" thread.
+
+Thanks to (at least)
+====================
+
+...
--
2.47.2

[Rob Browning]

Notice when one of the sources is exhausted and remove it from srcs;
exit when there are no more sources. This will be covered by a
forthcoming interactive test.

Signed-off-by: Rob Browning <r...@defaultvalue.org>
Tested-by: Rob Browning <r...@defaultvalue.org>
---

lib/bup/main.py | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/bup/main.py b/lib/bup/main.py
index 6f9c684b..f77de5ac 100755
--- a/lib/bup/main.py
+++ b/lib/bup/main.py
@@ -245,7 +245,7 @@ def filter_output(srcs, dests, control):

dest_for = dict(zip(srcs, dests))
pending = {}

try:
- while True:
+ while srcs:

ready_fds, _, _ = select.select(read_fds, [], [])

width = tty_width()
for fd in ready_fds:
@@ -253,6 +253,11 @@ def filter_output(srcs, dests, control):

continue
buf = os.read(fd, 4096)
dest = dest_for[fd]

+ if not buf:
+ srcs.remove(fd)
+ read_fds.remove(fd)
+ print_clean_line(dest, pending.pop(fd, []), width)
+ continue
split = sep_rx.split(buf)
while len(split) > 1:
content, sep = split[:2]
--
2.47.2