Closed Bug 463411 Opened 11 years ago Closed 10 years ago

nsinstall can race creating directories

Categories

(Firefox Build System :: General, defect)

x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: ted, Assigned: ted)

References

Details

Attachments

(1 file, 1 obsolete file)

/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.
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 :)
Depends on: 463417
Attached patch double-check with chdir (obsolete) — Splinter Review
Simple enough. Can't get this to fail with a little test shellscript. (nsinstall -D /foo & nsinstall -D /foo, in a loop)
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Attachment #409131 - Flags: review?(timeless)
Attachment #409131 - Flags: review?(timeless) → review+
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.
Summary: nsinstall can race when called with -D → nsinstall can race creating directories
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.)
Attachment #409131 - Attachment is obsolete: true
Attachment #409320 - Flags: review?(timeless)
Attachment #409320 - Flags: review?(timeless) → review+
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/e857761718d5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.