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.

Sign up for GitHub

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

Jump to bottom

Fix mkdir race condition in FileSystemCacheHandler #375

Merged
merged 4 commits into from
Dec 3, 2019
Merged

Fix mkdir race condition in FileSystemCacheHandler #375

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a2f59d2
Fix mkdir race condition in FileSystemCacheHandler
B-Galati Oct 14, 2019
574130b
review
B-Galati Oct 17, 2019
29ae0c5
replace exception by trigger_error E_USER_WARNING
B-Galati Dec 2, 2019
25dadd4
Merge branch 'master' into fix-mkdir-cache
joshcanhelp Dec 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Helpers/Cache/FileSystemCacheHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ class FileSystemCacheHandler implements CacheHandler
public function __construct($temp_directory_prefix = 'auth0-php')
{
$this->tmp_dir = sys_get_temp_dir().DIRECTORY_SEPARATOR.$temp_directory_prefix.DIRECTORY_SEPARATOR;
if (! file_exists($this->tmp_dir)) {
mkdir($this->tmp_dir);
if (! is_dir($this->tmp_dir) && ! @mkdir($this->tmp_dir, 0777, true) && ! is_dir($this->tmp_dir) ) {
B-Galati marked this conversation as resolved.
Show resolved Hide resolved
throw new \RuntimeException("Cache Handler was not able to create directory '$this->tmp_dir'");
Copy link
Contributor

@joshcanhelp joshcanhelp Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Are we sure we want to throw an exception here? Seems like we shouldn't fail the application if cache isn't running. Also, if we're increasing from a warning/notice that can be hidden/logged to a potential exception, that has the potential to break folks on a patch/minor upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, IMHO it sounds like a big problem to have a failing cache system and not being aware that it does not work.

Also, if we're increasing from a warning/notice that can be hidden/logged to a potential exception, that has the potential to break folks on a patch/minor upgrade.

At the moment the set method of the cache handler would fail if the tmp directory does not exist so it should not change anything for the next version. On the other hand, with this PR it would not create false positive warning for a non existent directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment the set method of the cache handler would fail

It would fail but it would be a warning, not a fatal or exception:

https://www.php.net/manual/en/function.fopen.php

I agree that a failing cache system is a problem and, in this case, since it's not something that runs on every load, it's probably best to stop the process. But we can't introduce an exception here in a minor or patch release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment the set method of the cache handler would fail

It would fail but it would be a warning, not a fatal or exception:

https://www.php.net/manual/en/function.fopen.php

Indeed! Thank you :-)

I agree that a failing cache system is a problem and, in this case, since it's not something that runs on every load, it's probably best to stop the process. But we can't introduce an exception here in a minor or patch release.

That is not a documented and should be catched exception. That is only a safe guard in case something is wrong that the user should be aware of. A bit like a NullPointerException or IndexOufOfBoundException that would be thrown by the runtime himself.

}
}

Expand Down