Skip to content

path: serialize iconv_open and iconv_close - #7404

Open
stig-codes wants to merge 1 commit into
libgit2:mainfrom
stig-codes:iconv-serialize
Open

stig-codes wants to merge 1 commit into
libgit2:mainfrom
stig-codes:iconv-serialize

Conversation

@stig-codes

Copy link
Copy Markdown

Fixes #7403.

On macOS 14 and later, iconv_open and iconv_close take a process-wide pthread rwlock that can lose a wakeup when a waiting thread takes a signal. Once that happens, every thread that opens or closes a converter blocks forever. We open and close a converter for every directory we iterate and every reference name we normalize whenever core.precomposeunicode is set, which is the default on APFS. So a multithreaded program that handles any signal can hang in a reference lookup, a status or a diff. #7403 has the details, the Apple source references and a reproducer.

This change holds a process-wide git_mutex around iconv_open and iconv_close. The mutex is created by git_fs_path_iconv_global_init, which git_libgit2_init runs alongside the other util-layer inits; it is freed through git_runtime_shutdown_register. With only one thread ever inside those calls, iconv's own lock never has a waiter. The conversions themselves (iconv()) don't take that lock and stay unserialized.

Notes for review:

  • git_fs_path_iconv_init_precompose now sets map to (iconv_t)-1 before it takes the lock, so a failed lock leaves ic in a state git_fs_path_iconv_clear accepts. A lock failure is reported as GIT_ERROR_THREAD, as in mwindow.c.
  • The lock call is checked for a non-zero return, as in mwindow.c and openssl.c. On POSIX, git_mutex_lock is pthread_mutex_lock, which returns a positive errno, so a < 0 check would never fire.
  • Builds without GIT_I18N_ICONV compile all of this out, including the init entry.
  • No test is added. The failure needs a signal to land on a thread that is waiting inside the OS's rwlock. It is timing-dependent and macOS-only, and a regression would hang the suite rather than fail it. The existing iconv, path, refs, iterator and threads tests exercise the lock, and tests/util/init.c exercises its init and shutdown.

Testing, on macOS 26.5.1 with an Apple M3 Ultra:

  • Reproducer from Threads hang forever in iconv_open/iconv_close on macOS 14+ when a signal arrives (core.precomposeunicode) #7403 (64 threads, SIGUSR1 every 200 µs). Unpatched main wedged in 3 of 3 runs, and in 5 of 5 runs of a longer harness. Patched main kept going for the full 20 s in 3 of 3 and 5 of 5 runs, and for a 300 s soak: 6.3M lookups under about 985k signals.
  • Backport. The same change applied to v1.9.7, with GIT_USE_ICONV in place of GIT_I18N_ICONV, went from 3 of 3 runs wedged to 3 of 3 kept going.
  • Test suites. util_tests and libgit2_tests -xonline pass. The util suite and the status::renames, refs, repo::init, iterator and threads suites give identical results with and without the patch, on main and on v1.9.7.
  • Build configurations. Builds warning-free with -DENABLE_WERROR=ON, with -DUSE_THREADS=OFF, with both together, and with -DUSE_I18N=OFF.
  • Throughput. Single-threaded throughput is unchanged, at about 75k lookups/s either way. With 64 threads on one repository, the patched build does about 22k lookups/s against about 75k unpatched. Unpatched libgit2 with precomposition off, which never calls iconv, also does about 22k/s. The unpatched figure is higher only because those threads spend about 95% of their samples parked on iconv's lock, which throttles their file I/O. The mutex adds no measurable cost.

Since macOS 14 the system iconv is FreeBSD's Citrus implementation,
whose iconv_open takes one process-wide pthread rwlock twice and whose
iconv_close takes it once (`ci_lock` in citrus/citrus_iconv.c).  A
thread that is waiting for a macOS pthread rwlock when a signal arrives
can lose its wakeup: every waiter then blocks forever, and no thread
holds the lock.

We open and close a converter for every directory that we iterate and
every reference name that we normalize whenever core.precomposeunicode
is set, which git and libgit2 set on APFS.  So a multithreaded program
that handles any signal - a SIGCHLD handler for its child processes is
enough - can hang forever in a reference lookup, a status, a diff, or
anything else that iterates the working directory.

Hold a process-wide mutex around iconv_open and iconv_close.  Only one
thread is then ever inside them, so iconv's lock never has a waiter and
there is no wakeup to lose.  The conversions themselves (iconv) do not
take that lock and are not serialized.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Threads hang forever in iconv_open/iconv_close on macOS 14+ when a signal arrives (core.precomposeunicode)

1 participant