463411 - nsinstall can race creating directories

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

/local/bin/make -C public export
make[5]: Entering directory `/d/build/obj-firefox-opt-libxul/content/canvas'
/local/bin/make -C public export
make[6]: Entering directory `/d/build/obj-firefox-opt-libxul/content/canvas/publ
ic'make[6]: Entering directory `/d/build/obj-firefox-opt-libxul/content/base/pub
lic'

/c/mozilla-build/moztools/bin/nsinstall -D ../../../dist/include/content/c/mozil
la-build/moztools/bin/nsinstall -D ../../../dist/include/content

/c/mozilla-build/moztools/bin/nsinstall -D ../../../dist/sdk/include
..\..\..\dist\include\content: Could not create the directory: File exists
make[6]: *** [../../../dist/include/content] Error 3

So the PARALLEL_DIRS patch and the followup patch from bug 462440 exposed a race condition in "nsinstall -D". You can see it in the code here:
http://hg.mozilla.org/mozilla-central/annotate/874aba8a9a8a/config/nsinstall.c#l351
and here (the version apparently shipped with MozillaBuild):
http://bonsai.mozilla.org/cvsblame.cgi?file=buildtools%2Fwindows%2Fsource%2Fshmsdos%2Fnsinstall.c&rev=&cvsroot=%2Fcvsroot&mark=157-159#124

Basically, those mkdir checks should check that errno != EEXIST, since if two "nsinstall -D" processes are spawned simultaneously, they can both pass the "directory does not exist" check, and then both run mkdir, with one succeeding, and one failing with EEXIST.

Comment 1

[timeless]

we're running in a loop which means we want to use chdir instead of checking EEXIST. if chdir works, then we can continue through our loop.

if we checked EEXIST, we'd still have to perform the chdir, which makes it a useless check :)

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: double-check with chdir

Simple enough. Can't get this to fail with a little test shellscript. (nsinstall -D /foo & nsinstall -D /foo, in a loop)

Comment 3

[(not currently active) Ted Mielczarek]

You can hit this without -D, since nsinstall.exe uses the same codepath to create destination directories for files.

Also, my previous patch is insufficient to fix this in all cases. New patch in a minute.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: double-check with chdir, the right way

This fixes the problem I was seeing. Two nsinstalls were racing doing something like:
nsinstall <a bunch of files> ../../../_tests/testing/mochitest/tests/a/b/c

The race was actually creating 'a', I think, but with my previous patch it would still fail because it was skipping that _wchdir(path), which gets you back to the cwd, so the next mkdir would fail. This patch works reliably for me. (I was reliably failing in the same spot when building with -j12 on an 8-core machine.)

Comment 5

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/e857761718d5