nsinstall can race creating directories

RESOLVED FIXED in mozilla1.9.3a1

Status

Firefox Build System
General
RESOLVED FIXED
10 years ago
2 months ago

People

(Reporter: ted, Assigned: ted)

Tracking

Trunk
mozilla1.9.3a1
x86
Windows XP
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
/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

10 years ago
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 :)
(Assignee)

Updated

10 years ago
Depends on: 463417
(Assignee)

Comment 2

9 years ago
Created attachment 409131 [details] [diff] [review]
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)
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
Attachment #409131 - Flags: review?(timeless)

Updated

9 years ago
Attachment #409131 - Flags: review?(timeless) → review+
(Assignee)

Comment 3

9 years ago
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
(Assignee)

Comment 4

9 years ago
Created attachment 409320 [details] [diff] [review]
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.)
Attachment #409131 - Attachment is obsolete: true
Attachment #409320 - Flags: review?(timeless)

Updated

9 years ago
Attachment #409320 - Flags: review?(timeless) → review+
(Assignee)

Comment 5

9 years ago
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/e857761718d5
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.