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

concurrency problem w/ plugin activation #6927

Closed
diosmosis opened this issue Jan 6, 2015 · 2 comments
Closed

concurrency problem w/ plugin activation #6927

diosmosis opened this issue Jan 6, 2015 · 2 comments
Assignees
@mnapoli
Labels
answered For when a question was asked and we referred to forum or answered it. Bug For errors / faults / flaws / inconsistencies etc.
Milestone
Backlog (Help wan...

Comments

@diosmosis
Copy link
Member

If multiple requests try to activate a plugin concurrently, OnDiskUIAsset::delete may throw. The situation that arises is as follows:

  1. one request starts the activation process, it checks whether the asset file exists
  2. another request checks whether it exists AND removes the file
  3. the original request tries to remove the file, but it doesn't exist anymore and an exception is thrown

To fix, OnDiskUIAsset should silence the unlink error and only throw if the file is not writable. This may require a bit of refactoring to the existing code.
@diosmosis diosmosis added the Bug For errors / faults / flaws / inconsistencies etc. label Jan 6, 2015
@mattab mattab added this to the Short term milestone Jan 6, 2015
@sgiehl sgiehl mentioned this issue Jan 9, 2015
Closed
@mnapoli
Copy link
Contributor

mnapoli commented Jan 11, 2015

How about something like this:

if (file_exists($file)) {
    $result = @unlink($file);
    if (!$result && file_exists($file)) {
        throw new Exception(...);
    }
}

i.e. check file_exists again after removing, that way if a concurrent request already removed the file, then the exception will not be thrown because the file will be gone anyway.

I also suggest we move those file handling methods to Piwik\Filesystem.

If you think that's a good solution I could work on a PR.

@diosmosis
Copy link
Member Author

I think that will work. At least I can't think of a way for it to fail.

@mnapoli mnapoli mentioned this issue Jan 13, 2015
Merged
@mnapoli mnapoli self-assigned this Jan 13, 2015
diosmosis added a commit that referenced this issue Jan 15, 2015
@diosmosis
Merge pull request #6990 from piwik/bug/6927
74b2956 
Fix #6927 race conditions when deleting files
@mattab mattab added the answered For when a question was asked and we referred to forum or answered it. label Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mnapoli mnapoli

Labels
answered For when a question was asked and we referred to forum or answered it. Bug For errors / faults / flaws / inconsistencies etc.
Projects
None yet
Milestone
Backlog (Help wanted)
Development

No branches or pull requests
3 participants
@diosmosis @mattab @mnapoli