Re: [coreutils] [PATCH] sort: fix some --compress reaper bugs

[Paul Eggert]

On 14/12/10 09:55, Jim Meyering wrote:
> It'd be a shame to make everyone sleep even one second for NFS.

Well, it is marked as an _expensive_ test.  :-)

On 12/14/10 02:07, Pádraig Brady wrote:
> If the subprocesses were reparented to the shell,
> then one could just `wait`.

I don't know how to do reparenting like that.  But this raises a
more general issue: is it OK that when 'sort' is interrupted and
exits, that it does not kill off its children?  That is, it relies on
compressors nicely exiting (because 'sort' exits and they read EOF),
and it relies on decompressors nicely exiting (because they write to a
dangling pipe and get SIGPIPE or a write failure).  If either
assumption is false, 'sort' will leave behind orphan processes that
never exit.

I'm sort of inclined to say that's OK, and that it's the invoker's
responsibility to make sure that compressors and decompressors behave
'nicely'.  There are other ways in which they have to be 'nice', too:
for example, they shouldn't go poking around the file system, fiddle
with all the file descriptors that they have open, etc.  This is all
obvious stuff but perhaps it should be documented?

Anyway, to get back to the topic, here's the change I pushed.  It's
less fancy than reparenting or looping or whatever, but it should
fix the problem in 99.9% of the time.  I'm not sure the remaining
0.1% is worth the tuning effort, given that it's an expensive test
anyway.

>From b5bf8f9d6492b7f60f95f0665dfcd35442dbcb28 Mon Sep 17 00:00:00 2001
From: Paul Eggert <address@hidden>
Date: Tue, 14 Dec 2010 10:07:36 -0800
Subject: [PATCH] tests: default to /tmp as the temporary directory

* tests/check.mk (TESTS_ENVIRONMENT): Default TMPDIR to /tmp,
rather than to the working directory; this is more common in
practice, which makes the tests more real-worldish; and it is
often faster.  Also, it avoids some problems with NFS cleanups.
* tests/misc/sort-compress: Remove unnecessary code setting TMPDIR.
* tests/misc/sort-compress-proc: Likewise.  Do the final sleep
only if TMPDIR is relative, which should be rarely given the
change to TESTS_ENVIRONMENT.
---
 tests/check.mk                |    3 ++-
 tests/misc/sort-compress      |    3 ---
 tests/misc/sort-compress-proc |   14 +++++---------
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/tests/check.mk b/tests/check.mk
index df0c924..1391d51 100644
--- a/tests/check.mk
+++ b/tests/check.mk
@@ -49,7 +49,8 @@ built_programs = \
 # variables to test scripts.
 TESTS_ENVIRONMENT =                            \
   . $(srcdir)/lang-default;                    \
-  tmp__=$$TMPDIR; test -d "$$tmp__" || tmp__=.;        \
+  tmp__=$${TMPDIR-/tmp};                       \
+  test -d "$$tmp__" && test -w "$$tmp__" || tmp__=.;   \
   . $(srcdir)/envvar-check;                    \
   TMPDIR=$$tmp__; export TMPDIR;               \
   exec 9>&2;                                   \
diff --git a/tests/misc/sort-compress b/tests/misc/sort-compress
index a51a2df..50845c9 100755
--- a/tests/misc/sort-compress
+++ b/tests/misc/sort-compress
@@ -22,9 +22,6 @@ print_ver_ sort
 seq -w 2000 > exp || framework_failure
 tac exp > in || framework_failure
 
-# Ensure that $TMPDIR is valid.
-TMPDIR=.; export TMPDIR
-
 # This should force the use of temp files
 sort -S 1k in > out || fail=1
 compare exp out || fail=1
diff --git a/tests/misc/sort-compress-proc b/tests/misc/sort-compress-proc
index 8d0094f..30f6b43 100755
--- a/tests/misc/sort-compress-proc
+++ b/tests/misc/sort-compress-proc
@@ -20,13 +20,6 @@
 print_ver_ sort
 expensive_
 
-# Ensure that $TMPDIR is valid.
-if test -d /tmp && test -w /tmp
-then TMPDIR=/tmp
-else TMPDIR=.
-fi
-export TMPDIR
-
 seq -w 2000 > exp || fail=1
 tac exp > in || fail=1
 insize=$(stat -c %s - <in) || fail=1
@@ -78,10 +71,13 @@ rm -f ok
 
 rm -f compress
 
-# Give compression subprocesses time to react when 'sort' exits.
+# If $TMPDIR is relative, give subprocesses time to react when 'sort' exits.
 # Otherwise, under NFS, when 'sort' unlinks the temp files and they
 # are renamed to .nfsXXXX instead of being removed, the parent cleanup
 # of this directory will fail because the files are still open.
-sleep 1
+case $TMPDIR in
+/*) ;;
+*) sleep 1;;
+esac
 
 Exit $fail
-- 
1.7.2