htsserver never returns from main(), it blocks on its own exit wait

background_threads counted two unrelated things: the pinger, which only ever
leaves through exit(), and each mirror thread, which does return. main() then
waited on background_threads - 1, so with no pinger it asked for a negative
number of outstanding threads and the counter cannot go there, and with one it
waited on the pinger itself. Either way htsserver hung instead of returning.

Count only what never returns, and let each wait name what it must leave
running: the pinger for main(), the pinger plus itself for back_launch_cmd().
Waiting on background_threads alone would have swapped the hang for an early
return, letting main() tear down while a mirror was still crawling.

Closes #753

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Xavier Roche <xroche@gmail.com>

.github/workflows/ci.yml

@@ -46,10 +46,32 @@ jobs:
       - name: Configure
         run: |
           set -euo pipefail
-          # Regenerate from configure.ac/Makefile.am to validate them; the
-          # committed generated files already let a plain checkout build.
+          # Regenerate: configure and the Makefile.in's are not tracked.
           autoreconf -fi
+          # Disabling zlib must fail here rather than at link with a pile of
+          # undefined minizip references (#735). Both spellings, so a rewrite
+          # cannot keep one and lose the other. Probed out-of-tree to leave
+          # nothing behind.
+          nozlib="$RUNNER_TEMP/nozlib"
+          for arg in --without-zlib --with-zlib=no; do
+            rm -rf "$nozlib" && mkdir -p "$nozlib"
+            if (cd "$nozlib" && "$GITHUB_WORKSPACE/configure" "$arg" >out.log 2>&1); then
+              echo "::error::configure $arg succeeded; it must be rejected"
+              exit 1
+            fi
+            # ... and for the stated reason, not an unrelated configure failure.
+            grep -q "zlib cannot be disabled" "$nozlib/out.log" \
+              || { cat "$nozlib/out.log"; exit 1; }
+          done
           ./configure
+          # Same dead end from the compile side. The bare compile is the
+          # control: without it a broken probe would pass vacuously.
+          hdr='#include "htsglobal.h"'
+          echo "$hdr" | $CC -I. -Isrc -fsyntax-only -xc -
+          if echo "$hdr" | $CC -DHTS_USEZLIB=0 -I. -Isrc -fsyntax-only -xc - 2>/dev/null; then
+            echo "::error::-DHTS_USEZLIB=0 compiled; htsglobal.h must reject it"
+            exit 1
+          fi
           # a missing decoder would silently drop the coding from Accept-Encoding
           grep -q "define HTS_USEBROTLI 1" config.h
           grep -q "define HTS_USEZSTD 1" config.h

configure.ac

@@ -175,7 +175,7 @@ AX_CHECK_ALIGNED_ACCESS_REQUIRED
 # check for various headers
 AC_CHECK_HEADERS([execinfo.h sys/ioctl.h])
 
-### zlib
+### zlib (mandatory)
 CHECK_ZLIB()
 
 ### brotli and zstd content codings (optional)

m4/check_zlib.m4

@@ -1,82 +1,33 @@
 dnl @synopsis CHECK_ZLIB()
 dnl
-dnl This macro searches for an installed zlib library. If nothing
-dnl was specified when calling configure, it searches first in /usr/local
-dnl and then in /usr. If the --with-zlib=DIR is specified, it will try
-dnl to find it in DIR/include/zlib.h and DIR/lib/libz.a. If --without-zlib
-dnl is specified, the library is not searched at all.
-dnl
-dnl If either the header file (zlib.h) or the library (libz) is not
-dnl found, the configuration exits on error, asking for a valid
-dnl zlib installation directory or --without-zlib.
-dnl
-dnl The macro defines the symbol HAVE_LIBZ if the library is found. You should
-dnl use autoheader to include a definition for this symbol in a config.h
-dnl file. Sample usage in a C/C++ source is as follows:
-dnl
-dnl   #ifdef HAVE_LIBZ
-dnl   #include <zlib.h>
-dnl   #endif /* HAVE_LIBZ */
-dnl
-dnl @version $Id$
-dnl @author Loic Dachary <loic@senga.org>
+dnl Look for zlib. It is a hard requirement, not an option: the cache and the
+dnl WARC output are zip/gzip containers, and the bundled minizip calls zlib
+dnl directly. --with-zlib=DIR points at a non-standard prefix.
 dnl
+dnl Adds -lz to LIBS and defines HAVE_LIBZ.
 
-AC_DEFUN([CHECK_ZLIB],
-#
-# Handle user hints
-#
-[AC_MSG_CHECKING(if zlib is wanted)
-AC_ARG_WITH(zlib,
-[  --with-zlib=DIR root directory path of zlib installation [defaults to
-                    /usr/local or /usr if not found in /usr/local]
-  --without-zlib to disable zlib usage completely],
-[if test "$withval" != no ; then
-  AC_MSG_RESULT(yes)
-  ZLIB_HOME="$withval"
-else
-  AC_MSG_RESULT(no)
-fi], [
-AC_MSG_RESULT(yes)
-ZLIB_HOME=/usr/local
-if test ! -f "${ZLIB_HOME}/include/zlib.h"
-then
-        ZLIB_HOME=/usr
+AC_DEFUN([CHECK_ZLIB], [
+AC_ARG_WITH([zlib],
+	[AS_HELP_STRING([--with-zlib=DIR],[root directory of the zlib installation])],
+	[zlib_want=$withval], [zlib_want=yes])
+if test "$zlib_want" = "no"; then
+	AC_MSG_ERROR([zlib cannot be disabled: the cache and the WARC output are zip/gzip containers, and the bundled minizip calls zlib directly])
 fi
-])
-
-#
-# Locate zlib, if wanted
-#
-if test -n "${ZLIB_HOME}"
-then
-        ZLIB_OLD_LDFLAGS=$LDFLAGS
-        ZLIB_OLD_CPPFLAGS=$LDFLAGS
-        LDFLAGS="$LDFLAGS -L${ZLIB_HOME}/lib"
-        CPPFLAGS="$CPPFLAGS -I${ZLIB_HOME}/include"
-        AC_LANG_SAVE
-        AC_LANG_C
-        AC_CHECK_LIB(z, inflateEnd, [zlib_cv_libz=yes], [zlib_cv_libz=no])
-        AC_CHECK_HEADER(zlib.h, [zlib_cv_zlib_h=yes], [zlib_cv_zlib_h=no])
-        AC_LANG_RESTORE
-        if test "$zlib_cv_libz" = "yes" -a "$zlib_cv_zlib_h" = "yes"
-        then
-                #
-                # If both library and header were found, use them
-                #
-                AC_CHECK_LIB(z, inflateEnd)
-                AC_MSG_CHECKING(zlib in ${ZLIB_HOME})
-                AC_MSG_RESULT(ok)
-        else
-                #
-                # If either header or library was not found, revert and bomb
-                #
-                AC_MSG_CHECKING(zlib in ${ZLIB_HOME})
-                LDFLAGS="$ZLIB_OLD_LDFLAGS"
-                CPPFLAGS="$ZLIB_OLD_CPPFLAGS"
-                AC_MSG_RESULT(failed)
-                AC_MSG_ERROR(either specify a valid zlib installation with --with-zlib=DIR or disable zlib usage with --without-zlib)
-        fi
+if test "$zlib_want" != "yes"; then
+	# An explicit prefix is authoritative: if the header is not under it,
+	# error rather than silently pick a system copy.
+	if test ! -f "$zlib_want/include/zlib.h"; then
+		AC_MSG_ERROR([zlib requested at $zlib_want but $zlib_want/include/zlib.h is missing])
+	fi
+	CPPFLAGS="$CPPFLAGS -I$zlib_want/include"
+	LDFLAGS="$LDFLAGS -L$zlib_want/lib"
+elif test -f /usr/local/include/zlib.h; then
+	# Where the BSD ports tree lands zlib, and not always searched by default.
+	CPPFLAGS="$CPPFLAGS -I/usr/local/include"
+	LDFLAGS="$LDFLAGS -L/usr/local/lib"
 fi
-
+AC_CHECK_HEADER([zlib.h], [],
+	[AC_MSG_ERROR([zlib.h not found; install the zlib development files or pass --with-zlib=DIR])])
+AC_CHECK_LIB([z], [inflateEnd], [],
+	[AC_MSG_ERROR([libz not found; install the zlib development files or pass --with-zlib=DIR])])
 ])

src/htsback.c

@@ -48,11 +48,6 @@ Please visit our Website: http://www.httrack.com
 #include "htsftp.h"
 #include "htscodec.h"
 #include "htsproxy.h"
-#if HTS_USEZLIB
-#include "htszlib.h"
-#else
-#error HTS_USEZLIB not defined
-#endif
 
 #ifdef _WIN32
 #ifndef __cplusplus

src/htsglobal.h

@@ -138,10 +138,11 @@ Please visit our Website: http://www.httrack.com
 #define HTS_DOSNAME 0
 #endif
 
-// utiliser zlib?
+// zlib is mandatory: the cache is a zip and minizip calls it regardless
 #ifndef HTS_USEZLIB
-// autoload
 #define HTS_USEZLIB 1
+#elif !HTS_USEZLIB
+#error HTS_USEZLIB=0 is not a supported configuration
 #endif
 
 // brotli and zstd content codings; off unless the build opted in (configure,

src/htsthread.c

@@ -44,9 +44,18 @@ Please visit our Website: http://www.httrack.com
 #endif
 #endif
 
+/* Outstanding threads, counted at spawn rather than by the child at entry, so
+   that a caller which spawns and immediately waits still joins them (#747). */
 static int process_chain = 0;
 static htsmutex process_chain_mutex = HTSMUTEX_INIT;
 
+static void process_chain_add(int delta) {
+  hts_mutexlock(&process_chain_mutex);
+  process_chain += delta;
+  assertf(process_chain >= 0);
+  hts_mutexrelease(&process_chain_mutex);
+}
+
 HTSEXT_API void htsthread_wait(void) {
   htsthread_wait_n(0);
 }
@@ -99,20 +108,12 @@ static void *hts_entry_point(void *tharg)
   void *const arg = s_args->arg;
   void (*fun) (void *arg) = s_args->fun;
 
-  free(tharg);
-
-  hts_mutexlock(&process_chain_mutex);
-  process_chain++;
-  assertf(process_chain > 0);
-  hts_mutexrelease(&process_chain_mutex);
+  freet(tharg);
 
   /* run */
   fun(arg);
 
-  hts_mutexlock(&process_chain_mutex);
-  process_chain--;
-  assertf(process_chain >= 0);
-  hts_mutexrelease(&process_chain_mutex);
+  process_chain_add(-1);
 #ifdef _WIN32
   return 0;
 #else
@@ -122,18 +123,20 @@ static void *hts_entry_point(void *tharg)
 
 /* create a thread */
 HTSEXT_API int hts_newthread(void (*fun) (void *arg), void *arg) {
-  hts_thread_s *s_args = malloc(sizeof(hts_thread_s));
+  hts_thread_s *s_args = malloct(sizeof(hts_thread_s));
 
   assertf(s_args != NULL);
   s_args->arg = arg;
   s_args->fun = fun;
+  process_chain_add(1);
 #ifdef _WIN32
   {
     unsigned int idt;
     HANDLE handle =
       (HANDLE) _beginthreadex(NULL, 0, hts_entry_point, s_args, 0, &idt);
     if (handle == 0) {
-      free(s_args);
+      process_chain_add(-1);
+      freet(s_args);
       return -1;
     } else {
       /* detach the thread from the main process so that is can be independent */
@@ -151,7 +154,8 @@ HTSEXT_API int hts_newthread(void (*fun) (void *arg), void *arg) {
         || pthread_attr_setstacksize(&attr, stackSize) != 0
         || (retcode =
             pthread_create(&handle, &attr, hts_entry_point, s_args)) != 0) {
-      free(s_args);
+      process_chain_add(-1);
+      freet(s_args);
       return -1;
     } else {
       /* detach the thread from the main process so that is can be independent */

src/htsweb.c

@@ -101,8 +101,8 @@ static void htsweb_sig_brpipe(int code) {
   /* ignore */
 }
 
-/* Number of background threads */
-static int background_threads = 0;
+/* Threads that never return; no wait may count on them draining. */
+static int nonjoinable_threads = 0;
 
 /* Server/client ping handling */
 static htsmutex pingMutex = HTSMUTEX_INIT;
@@ -299,15 +299,19 @@ int main(int argc, char *argv[]) {
 
   /* pinger */
   if (parentPid > 0) {
-    hts_newthread(client_ping, (void *) (uintptr_t) parentPid);
-    background_threads++; /* Do not wait for this thread! */
+    if (hts_newthread(client_ping, (void *) (uintptr_t) parentPid) == 0) {
+#ifndef _WIN32
+      nonjoinable_threads++; /* client_ping() only ever leaves through exit() */
+#endif
+    }
     smallserver_setpinghandler(pingHandler, NULL);
   }
 
   /* launch */
   ret = help_server(argv[1], defaultPort, bindAddr);
 
-  htsthread_wait_n(background_threads - 1);
+  /* Drain everything a mirror may still have in flight, the pinger aside. */
+  htsthread_wait_n(nonjoinable_threads);
   hts_uninit();
 
 #ifdef _WIN32
@@ -382,7 +386,6 @@ void webhttrack_main(char *cmd) {
   commandRunning = 1;
   DEBUG(fprintf(stderr, "commandRunning=1\n"));
   hts_newthread(back_launch_cmd, (void *) strdup(cmd));
-  background_threads++; /* Do not wait for this thread! */
 }
 
 void webhttrack_lock(void) {
@@ -423,8 +426,8 @@ static int webhttrack_runmain(httrackp * opt, int argc, char **argv) {
   /* Rock'in! */
   ret = hts_main2(argc, argv, opt);
 
-  /* Wait for pending threads to finish */
-  htsthread_wait_n(background_threads);
+  /* Wait for pending threads to finish; the pinger and this thread stay. */
+  htsthread_wait_n(nonjoinable_threads + 1);
 
   return ret;
 }

src/httrack.c

@@ -306,8 +306,8 @@ int main(int argc, char **argv) {
     fprintf(stderr, "* %s\n", hts_errmsg(opt));
   }
   global_opt = NULL;
+  htsthread_wait(); /* pending threads still read opt */
   hts_free_opt(opt);
-  htsthread_wait();             /* wait for pending threads */
   hts_uninit();
 
 #ifdef _WIN32

tests/65_port-siblings.test

@@ -79,6 +79,29 @@ done
 # 65535 is a valid port the old "< 65535" bound refused
 web_accepted 65535
 
+# --- htsserver returns from main() instead of blocking at exit -------------
+# The exit wait must exclude exactly the threads that never return (#753).
+# $tmp has no lang.def, so the server fails right after announcing and main()
+# reaches the wait on its own; rc, not EXITED, carries the verdict.
+
+web_exits() {
+    local rc=0
+
+    : >"$tmp/exit.log"
+    run_with_timeout 30 htsserver "$tmp" "$@" >"$tmp/exit.log" 2>&1 || rc=$?
+    grep -q "^EXITED" "$tmp/exit.log" ||
+        ! echo "FAIL: #753: htsserver ${*:-(no options)} never finished serving" || exit 1
+    # exactly 1, not merely "not 124": a crash or an assertf abort also escapes
+    # the wait, and would otherwise read as a pass
+    test "$rc" -eq 1 ||
+        ! echo "FAIL: #753: htsserver ${*:-(no options)} exited $rc, want 1" || exit 1
+}
+
+# no pinger: the excluded count went negative
+web_exits
+# a pinger, which never returns and so must stay excluded
+web_exits --ppid $$
+
 # --- proxytrack <proxy-addr:port> <ICP-addr:port> --------------------------
 # A bad argument falls through to the usage screen; it had no range check at
 # all, so 65616 quietly listened on port 80. A valid one binds and blocks.