-
Notifications
You must be signed in to change notification settings - Fork 81
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Race condition in Shell.mkdir_p #300
Comments
Thanks for the report. A very safe thing to be doing would be to replace It's actually easy to reproduce this failure, using a makefile that looks as follows: DIR=foo
FILE=code.ml
setup:
mkdir -p $(DIR)
echo "let x = 1" > $(DIR)/$(FILE)
echo "run 'make -j all' to reproduce the failure'"
clean:
ocamlbuild -clean
dist-clean: clean
rm $(DIR)/$(FILE)
rmdir $(DIR)
.PHONY: all test1 test2
all: test1 test2
test1:
$(MAKE) _build/$(DIR)/$(FILE)
test2:
$(MAKE) _build/$(DIR)/$(FILE)
_build/$(DIR)/$(FILE):
ocamlbuild -no-log $(DIR)/$(FILE) I will first test the safe-and-incomplete change; if it fixes the reproduction case, maybe that's a good first step and I can do a minor release. Then I will look at the harder-and-complete fix. |
So:
|
As far as I can tell (but not execute right now), you can simply use |
Yes, but the most important behavior for the present issue is what happens in case the directory or path already exists (not to fail in that case), which I didn't find documented. I'm also not completely sure of how to know which In any case, I changed the implementation in the PR a bit from the previous message: now what I do is that I just ignore any failure of |
ocamlbuild/src/shell.ml
Lines 62 to 64 in 4ef4b18
If another process creates the directory after the
sys_file_exists
call but before themkdir
, thenmkdir
raises.This has been happening in the wild for Flow (facebook/flow#8027). I suspect it's because of
make -j
and we have multipleocamlbuild
targets so they run in parallel.A safer approach could be to suppress the
EEXIST
: https://github.com/janestreet/core/blob/e8cf8a6d340c4af1e8177591782fb3e326eb0a0b/src/core_unix.ml#L1463-L1478The text was updated successfully, but these errors were encountered: