Skip to content

Conversation

kcassam
Copy link
Contributor

@kcassam kcassam commented Aug 21, 2014

[Filesystem Component] Fix mkdir race condition

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #11626
License MIT
Doc PR none

@kcassam
Copy link
Contributor Author

kcassam commented Aug 21, 2014

I made a pull request into master instead of 2.3 by error.
I just corrected this mistake.

For information, the bad PR is here #11697

@kcassam
Copy link
Contributor Author

kcassam commented Aug 21, 2014

Can somebody help me with travis failure ? What should I do ?

throw new IOException(sprintf('Failed to create %s', $dir));
if (!is_dir($dir)) {
// The directory was not created by a concurrent process. Let's throw an exception with a developer friendly error message
$error = error_get_last();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could move this line before the is_dir check, just to be sure that you really get the error from mkdir and not from is_dir

the condition should then be if ($error && !is_dir($dir))

@jakzal
Copy link
Contributor

jakzal commented Aug 21, 2014

@kcassam travis fails because you gave wrong number of arguments to the constructor. It only has 3 arguments, like the main PHP exception class.

travis and other comments have been taken into account.
@kcassam
Copy link
Contributor Author

kcassam commented Aug 21, 2014

Thanks @jakzal @nicolas-grekas, i reworked my changes

@jakzal
Copy link
Contributor

jakzal commented Aug 21, 2014

👍

@1emming
Copy link
Contributor

1emming commented Aug 21, 2014

Cool fix :)
One question though. In the code of the last commit when the make dir fails but no error can be found and there still is no directory then no exception is thrown (but it should). If this cannot happen because there always will be an error then the error doesn`t need to be checked. If we cannot rely on an error being there then an exception should be thrown without the error message.

@kcassam
Copy link
Contributor Author

kcassam commented Aug 21, 2014

Question : Who will do the adaptation for version >= 2.4 because IOException constructor call are not the same :

//2.3 : 
throw new IOException(sprintf('Failed to create %s', $dir));

// >= 2.4 : 
throw new IOException(sprintf('Failed to create "%s".', $dir), 0, null, $dir);

Should I do another pull request ?

@kcassam
Copy link
Contributor Author

kcassam commented Aug 21, 2014

I agree with @1emming #11726 (comment)

I suggest

Simpler :

if (true !== @mkdir($dir, $mode, true)) {
    if (!is_dir($dir)) {
        // The directory was not created by a concurrent process.
        throw new IOException(sprintf('Failed to create "%s"', $dir));
    }
}

Or

More developer friendly :

if (true !== @mkdir($dir, $mode, true)) {
    $error = error_get_last();
    if (!is_dir($dir)) {
        if ($error) {
            // The directory was not created by a concurrent process. Let's throw an exception with a developer friendly error message
            throw new IOException(sprintf('Failed to create "%s": %s.', $dir, $error['message']));
        }
        throw new IOException(sprintf('Failed to create "%s"', $dir));
    }
}

Which is better ?

better error management following feedback
@kcassam
Copy link
Contributor Author

kcassam commented Aug 25, 2014

I commited the developer friendly version

throw new IOException(sprintf('Failed to create %s', $dir));
$error = error_get_last();
if (!is_dir($dir)) {
// The directory was not created by a concurrent process. Let's throw an exception with a developer friendly error message if we have one
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a very minor issue: one space is missing at the beginning of this line.

@kcassam kcassam changed the title #11626 race condition fix [Filesystem Component] mkdir race condition fix #11626 Aug 25, 2014
@fabpot
Copy link
Member

fabpot commented Aug 27, 2014

👍

@fabpot
Copy link
Member

fabpot commented Aug 27, 2014

Thank you @kcassam.

fabpot added a commit that referenced this pull request Aug 27, 2014
…assam)

This PR was squashed before being merged into the 2.3 branch (closes #11726).

Discussion
----------

[Filesystem Component] mkdir race condition fix #11626

[Filesystem Component] Fix mkdir race condition

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #11626
| License       | MIT
| Doc PR        | none

Commits
-------

0483452 [Filesystem Component] mkdir race condition fix #11626
@fabpot fabpot closed this Aug 27, 2014
@kcassam
Copy link
Contributor Author

kcassam commented Aug 27, 2014

Happy to have contributed. One newbie question though : How this correction will go through version of Symfony >=2.4 to master ? Do I have to redo it for each version ?

@fabpot
Copy link
Member

fabpot commented Aug 27, 2014

@kcassam Thanks for your help. You don't need to do anything; old branches are merged into newer ones on a regular basis. So, your changes will appear in other branches in the next days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants