From f0ec725234463a7165a4339c8a65486a9b04ab3a Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 5 Oct 2020 13:53:43 -0400 Subject: [PATCH] include buildid in LOCPATH This avoids the possibility that the bundle could be updated in place, leading to LOCPATH existing but containing locales for the old version, which needed to be tested for with code that was not race-free. LOCPATH/buildid is still written and checked when cleaning up stale caches. That is not actually necessary, except old versions of the standalone bundle expect to see it, and this prevents them cleaning up the locale cache of a new version. And still checking it prevents the new version cleaning up the locale cache of the old version while the old version is still in use. Added explicit tests before creating LOCPATH and the base and buildid files. The buildid file no longer needs to be updated every time, because it's stable for the given LOCPATH directory. And the base file actually did not need to be updated every time, because the LOCPATH is derived from base, so if the bundle is moved elsewhere, a different LOCPATH will be used. Transitioning to this will mean that two git-annex builds that otherwise have the same buildid -- the same git-annex md5sum -- will use different LOCPATH values, but that's handled fine by the cache cleanup code, so at most it will mean one extra generation of the locale files. --- ..._5c2c6003b45d78b9e0bee95f5a12c48c._comment | 37 +++++++++++++++++++ standalone/linux/skel/runshell | 32 ++++++++-------- 2 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 doc/bugs/standalone_runshell_can_race_and_fail_to_remove___96____126____47__.cache__47__git-annex__47__locales__47____96___dirs/comment_4_5c2c6003b45d78b9e0bee95f5a12c48c._comment diff --git a/doc/bugs/standalone_runshell_can_race_and_fail_to_remove___96____126____47__.cache__47__git-annex__47__locales__47____96___dirs/comment_4_5c2c6003b45d78b9e0bee95f5a12c48c._comment b/doc/bugs/standalone_runshell_can_race_and_fail_to_remove___96____126____47__.cache__47__git-annex__47__locales__47____96___dirs/comment_4_5c2c6003b45d78b9e0bee95f5a12c48c._comment new file mode 100644 index 0000000000..8e59c3c287 --- /dev/null +++ b/doc/bugs/standalone_runshell_can_race_and_fail_to_remove___96____126____47__.cache__47__git-annex__47__locales__47____96___dirs/comment_4_5c2c6003b45d78b9e0bee95f5a12c48c._comment @@ -0,0 +1,37 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 4""" + date="2020-10-05T17:04:45Z" + content=""" +Hmm, that rm -rf is idempotent itself once the stderr is hidden; +if a race causes one rm to fail the other one will probably succeed; +if both manage to fail it ignores the nonzero exit code and, +the next run will clean up after it by running it again. AFAICS the cache +eventually gets cleaned up in all circumstances. + + rm: cannot remove '/home/yoh/.cache/git-annex/locales/bb375eb6ec0d2706f1723307f068911a': Directory not empty + +I think what happened there is, the first process created +that cache directory, but had not yet written the base file +when the second process ran. So the second process sees what looks like a +cache directory with no base file, so it decides to clean it up. In the +meantime the first process has written base and other files and so the rm +fails. Also, the first process may succeed and end up running git-annex +with some locale files missing (if the rm happened to delete those), +resulting in incompatable system locales being used. + +So, it ought to defer cleaning up old caches until after it's made sure its +current cache is all set up. Then that race goes away. + +But, there's a problem with this, that comes right after the main cache +cleanup: + + # If the locale cache for this bundle is out of date, refresh it. + if [ -e "$LOCPATH/buildid" ] && ! cmp "$LOCPATH/buildid" "$base/buildid" >/dev/null 2>&1 ; then + rm -rf "$LOCPATH" + fi + +That runs if the git-annex.linux directory gets overwritten with a new version, +so the cache is the same. And it's also prone to similar races. +I think, to fix this, it probably needs to use the buildid in the LOCPATH. +"""]] diff --git a/standalone/linux/skel/runshell b/standalone/linux/skel/runshell index 90f262ca65..c63b05413c 100755 --- a/standalone/linux/skel/runshell +++ b/standalone/linux/skel/runshell @@ -133,7 +133,7 @@ unset LD_PRELOAD ORIG_LOCPATH="$LOCPATH" export ORIG_LOCPATH if [ -z "${LOCPATH+set}" ] && [ -z "$GIT_ANNEX_PACKAGE_INSTALL" ]; then - LOCPATH="$HOME/.cache/git-annex/locales/$(echo "$base" | tr / _ )" + LOCPATH="$HOME/.cache/git-annex/locales/$(cat "$base/buildid")_$(echo "$base" | tr / _ )" # try to generate a short version, if md5sum is available locpathmd5=$( (echo "$LOCPATH" | md5sum | cut -d ' ' -f 1 2>/dev/null) || true) if [ -n "$locpathmd5" ]; then @@ -149,24 +149,22 @@ if [ -z "${LOCPATH+set}" ] && [ -z "$GIT_ANNEX_PACKAGE_INSTALL" ]; then fi done - # If the locale cache for this bundle is out of date, refresh it. - if [ -e "$LOCPATH/buildid" ] && ! cmp "$LOCPATH/buildid" "$base/buildid" >/dev/null 2>&1 ; then - rm -rf "$LOCPATH" + if [ ! -d "$LOCPATH" ]; then + if ! mkdir -p "$LOCPATH"; then + echo "Unable to write to $LOCPATH; can't continue!" >&2 + exit 1 + fi fi - if ! mkdir -p "$LOCPATH"; then - echo "Unable to write to $LOCPATH; can't continue!" >&2 - exit 1 + if [ ! -e "$LOCPATH/base" ]; then + echo "$base" > "$LOCPATH/base.$$" + mv -f "$LOCPATH/base.$$" "$LOCPATH/base" + fi + # Not using cp to avoid using the one bundled with git-annex + # before the environment is set up to run it. + if [ ! -e "$LOCPATH/buildid" ]; then + cat < "$base/buildid" > "$LOCPATH/buildid.$$" + mv -f "$LOCPATH/buildid.$$" "$LOCPATH/buildid" fi - # This is updated each time, because the bundle could be moved to a - # different directory. - echo "$base" > "$LOCPATH/base.$$" - mv -f "$LOCPATH/base.$$" "$LOCPATH/base" - # This is updated each time, because the bundle could be updated - # in place to a new version. - # Not using cp to avoid using the one bundled with git-annex before - # the environment is set up to run it. - cat < "$base/buildid" > "$LOCPATH/buildid.$$" - mv -f "$LOCPATH/buildid.$$" "$LOCPATH/buildid" # Generate locale definition files for the locales in use, # using the localedef and locale files from the bundle.