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

Issue #9318: catch exceptions in SCSSCacher::resetCache() #9352

Merged
merged 1 commit into from
May 3, 2018

Conversation

Cybso
Copy link

@Cybso Cybso commented Apr 30, 2018

Fixes hard fails when SCSSCacher::resetCache() is not able to delete a cached file, as described in issue #9318 after an upgrade from Nextcloud 13.0.0.0 to 13.0.0.2.

@codecov
Copy link

codecov bot commented Apr 30, 2018

Codecov Report

Merging #9352 into master will increase coverage by 0.72%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #9352      +/-   ##
============================================
+ Coverage      51.2%   51.93%   +0.72%     
+ Complexity    25580    25395     -185     
============================================
  Files          1563     1608      +45     
  Lines         88054    95447    +7393     
  Branches          0     1393    +1393     
============================================
+ Hits          45087    49566    +4479     
- Misses        42967    45881    +2914
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/SCSSCacher.php 71.62% <100%> (-0.8%) 39 <4> (+1)
settings/Controller/LogSettingsController.php 0% <0%> (-30%) 1% <0%> (-2%)
apps/admin_audit/lib/AppInfo/Application.php 8.39% <0%> (-5.18%) 20% <0%> (-2%)
lib/private/Log/File.php 56.04% <0%> (-4.19%) 30% <0%> (-1%)
apps/files_trashbin/lib/Trashbin.php 72.46% <0%> (-0.25%) 136% <0%> (ø)
...e/AppFramework/DependencyInjection/DIContainer.php 82.8% <0%> (-0.08%) 54% <0%> (ø)
lib/private/Log/Rotate.php 0% <0%> (ø) 4% <0%> (+2%) ⬆️
apps/dav/appinfo/routes.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
...dmin_audit/composer/composer/autoload_classmap.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
... and 106 more

$file->delete();
try {
$file->delete();
} catch(NotPermittedException $e) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess it is not the NotPermittedException that gets thrown here... (I know the phpdoc says that is what it throws). But I guess it fails on some uncatched thing deep in the code.

Better catch a bit more generic here I think

@rullzer rullzer requested review from icewind1991 and juliusknorr May 2, 2018 07:26
@rullzer rullzer added bug 3. to review Waiting for reviews labels May 2, 2018
@rullzer rullzer added this to the Nextcloud 14 milestone May 2, 2018
@Cybso
Copy link
Author

Cybso commented May 2, 2018

@rullzer Have a look at the stack trace in #9318 : Actually it is NotPermittedException that is thrown here, although I wasn't able to determine the root cause because the problem was not reproducible once the cache has been resetted (I've tried to reset the ownership and mode of the files in the data directory before I've modified the code, of course - it didn't solved the problem). Nevertheless, other people reported the same exception in SCSSCacher::resetCache:

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

ok then lets do this!

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels May 2, 2018
Signed-off-by: Roland Tapken <roland@bitarbeiter.net>
@juliusknorr juliusknorr force-pushed the 9318_SCSSCacher_resetCache_catch_exception branch from 0d47624 to 6270a01 Compare May 2, 2018 19:23
@juliusknorr
Copy link
Member

rebased to trigger ci again

@MorrisJobke MorrisJobke merged commit 0f82107 into master May 3, 2018
@MorrisJobke MorrisJobke deleted the 9318_SCSSCacher_resetCache_catch_exception branch May 3, 2018 07:56
@MorrisJobke
Copy link
Member

@Cybso Mind to open a backport PR? Just base a branch on stable13, cherry-pick the commit from there onto it and then create a PR on GitHub against stable13.

@skjnldsv skjnldsv added the feature: caching Related to our caching system: scssCacher, jsCombiner... label May 3, 2018
Cybso pushed a commit that referenced this pull request May 3, 2018
…tch_exception

Issue #9318: catch exceptions in SCSSCacher::resetCache()

(cherry picked from commit 0f82107)
@Cybso
Copy link
Author

Cybso commented May 3, 2018

@MorrisJobke I tried in #9376, but I think I accidentally merged all changes in master...?!

Here's what I did:

git pull
git checkout stable13
git checkout -b 9318_SCSSCacher_resetCache_catch_exception_stable13
git cherry-pick -m 1 -x 0f8210792ce4bb786bed9fe09b97a4ec9f4d848e
git push origin 9318_SCSSCacher_resetCache_catch_exception_stable13

@MorrisJobke
Copy link
Member

@Cybso Just use the commit sha from this PR:

git checkpick 6270a01cab3d6cce864b8b795749598f9d65b6c3

That then should work ;)

@Cybso
Copy link
Author

Cybso commented May 3, 2018

Ok, I think I got it right, now :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: caching Related to our caching system: scssCacher, jsCombiner...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants