Skip to content
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

Concurrency directory creation #1678

Closed
Legion112 opened this issue May 26, 2022 · 14 comments
Closed

Concurrency directory creation #1678

Legion112 opened this issue May 26, 2022 · 14 comments
Labels
Milestone

Comments

@Legion112
Copy link
Contributor

Monolog version 2.4.0

We run test in parallel in sometimes we experience following behaviour:

 There is no existing directory at "/php-tests/report/logs/e2e/PathToTestDir" and it could not be cr  
  eated: File exists   

This is from \Monolog\Handler\StreamHandler::createDir

@Legion112 Legion112 added the Bug label May 26, 2022
@Seldaek
Copy link
Owner

Seldaek commented Jun 8, 2022

Oh we did check for concurrency but I think there is a missing clearstatcache() in there to properly fix this issue. I'll fix this thanks for reporting 👍🏻

@Seldaek
Copy link
Owner

Seldaek commented Jun 8, 2022

Hm no sorry.. PHP shouldn't cache the fact that a file is missing, so I don't quite see how this code can fail

set_error_handler([$this, 'customErrorHandler']);
$status = mkdir($dir, 0777, true);
restore_error_handler();
if (false === $status && !is_dir($dir)) {
throw new \UnexpectedValueException(sprintf('There is no existing directory at "%s" and it could not be created: '.$this->errorMessage, $dir));
}

If mkdir failed, it checks for is_dir again and if the failure reason is File exists then it should definitely be there.

Is it possible that your concurrent test runs create the dir, log, and then delete the dir very quickly so you can run into a race condition where mkdir fails but is_dir still returns false as it was deleted already?

If so maybe we could fix this by retrying if it is still missing and the error is File exists I guess.

@Legion112
Copy link
Contributor Author

Legion112 commented Jun 9, 2022

Retry seem like a realy good option here @Seldaek.
Your idea is brilliant

@Seldaek
Copy link
Owner

Seldaek commented Jun 9, 2022

@Legion112 I'm not sure this fixes anything in fact.. As per my comment on #1683 (comment) - I believe you may then run into another race condition whereas the directory gets wiped by another process and then the file write fails. The only real solution if you do clear the directory after the test is to run tests in different directories IMO.

@Legion112
Copy link
Contributor Author

@Seldaek well let's check if $this->errorMessage === 'File exists' and do not throw exception in that case?
What do you think?

@Seldaek
Copy link
Owner

Seldaek commented Jun 9, 2022

Maybe.. but if it exists why did is_dir() return false at line 196? Do you have a reliable way to reproduce the error so that we can try to inspect things further?

@Legion112
Copy link
Contributor Author

#1685

@Legion112
Copy link
Contributor Author

I don't know why is_dir returned false.
I understand that is strange case.
Maybe series of retry would prevent this issue.
Like try to run createDir body code 3 times only after 3 error throw exception.

@Seldaek
Copy link
Owner

Seldaek commented Jun 9, 2022

These are all just band-aids though, not fixes. I would really like to understand what is going on before doing anything that hides the problem.

@Legion112
Copy link
Contributor Author

Legion112 commented Jun 9, 2022

We could try to look at is_dir
Several users have pointed that can return false in same cases.
image

@Seldaek
Copy link
Owner

Seldaek commented Jun 9, 2022

What environment are you running this on? VM? virtual filesystem? etc?

@Legion112
Copy link
Contributor Author

Legion112 commented Jun 9, 2022

Docker container.
In AWS kubernetus.
Filesystem: OverlayFS

@Legion112
Copy link
Contributor Author

Also in the documentation for is_dir
image

@Seldaek
Copy link
Owner

Seldaek commented Jun 9, 2022

The stat cache is only for true result, so if is_dir returns true it will cache it, if false it doesn't cache, so that can't be it.

k8s / docker yeah sounds like that shouldn't cause weird issues like VMs but who knows.

@Seldaek Seldaek modified the milestones: 1.x, 2.x Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants