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

Remove base url from global cache prefix #8716

Merged
merged 5 commits into from
Mar 8, 2018
Merged

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Mar 7, 2018

Having the base url in the global cache prefix seems to have caused the various issues with the SCSSCacher/JSCombiner. The cache was not cleared properly when using multiple domains and for cases when the base url was fetched while running form the cli.

This PR moves the base url from the global prefix to the cache prefix itself, so when calling the clear method of any ICache, all entries will properly get removed.

It will also reduce the number of used cache keys, since we don't have duplicate keys for every base url now anymore.

Fix for #8705 - I think we should merge it in addition to #8714 since this will fix the root cause, that the cache was not cleared properly.

@MorrisJobke
Copy link
Member

This still gives the full deps in my env and doesn't clear them properly. :/ Let me check the prefixes on CLI and web.

@MorrisJobke
Copy link
Member

This still gives the full deps in my env and doesn't clear them properly. :/ Let me check the prefixes on CLI and web.

I needed to configure overwritehost to be localhost:8000 because on the CLI it was only localhost.

Having this set up the first request to that URL still results in a 404 and the followup requests show the proper file. :/ Still weird.

@MorrisJobke
Copy link
Member

The weird thing: the file is properly rendered, but somewhere later a 404 is triggered:

{
    "reqId": "8eRMrhk6ACNMqfkALk0m",
    "level": 0,
    "time": "2018-03-07T13:07:37+00:00",
    "remoteAddr": "172.20.0.1",
    "user": "admin",
    "app": "no app in context",
    "method": "GET",
    "url": "/index.php/js/core/merged-template-prepend.js?v=b2f9666b-0",
    "message": "JSCombiner: successfully cached: merged-template-prepend.js",
    "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/604.5.6 (KHTML, like Gecko) Version/11.0.3 Safari/604.5.6",
    "version": "14.0.0.1"
}

@MorrisJobke
Copy link
Member

404 page is rendered here:

return new NotFoundResponse();

@codecov
Copy link

codecov bot commented Mar 7, 2018

Codecov Report

Merging #8716 into master will decrease coverage by 24.71%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #8716       +/-   ##
============================================
- Coverage     31.43%   6.72%   -24.72%     
  Complexity    25368   25368               
============================================
  Files          1607    1607               
  Lines         95157   95153        -4     
  Branches       1379    1379               
============================================
- Hits          29917    6399    -23518     
- Misses        65240   88754    +23514
Impacted Files Coverage Δ Complexity Δ
lib/private/Template/SCSSCacher.php 0% <0%> (-71.54%) 36 <0> (ø)
lib/private/Template/JSCombiner.php 0% <0%> (-87.5%) 31 <0> (ø)
lib/private/Server.php 1.76% <0%> (-15.92%) 282 <0> (ø)
apps/theming/lib/ThemingDefaults.php 0% <0%> (-91.05%) 46 <0> (ø)
lib/private/TemplateLayout.php 0% <0%> (ø) 49 <0> (ø) ⬇️
apps/dav/lib/CalDAV/Activity/Setting/Event.php 0% <0%> (-100%) 8% <0%> (ø)
.../Contacts/ContactsMenu/Providers/EMailProvider.php 0% <0%> (-100%) 4% <0%> (ø)
core/Controller/ContactsMenuController.php 0% <0%> (-100%) 4% <0%> (ø)
lib/private/Security/CSRF/CsrfTokenGenerator.php 0% <0%> (-100%) 2% <0%> (ø)
...eLimiting/Exception/RateLimitExceededException.php 0% <0%> (-100%) 1% <0%> (ø)
... and 622 more

@MorrisJobke
Copy link
Member

The weird thing: the file is properly rendered, but somewhere later a 404 is triggered:

Not that weird ... this code is called before the "isCached" is called and thus fails to fetch it. Maybe there is another position where we should check for the file first.

@MorrisJobke
Copy link
Member

@juliushaertl Still the same behavior with the most recent commit.

@MorrisJobke
Copy link
Member

Okay - so the files are regenerated on the processing of the 404 template and not on the call to JsController::getJs method. Thus it fails on the first request and then is cached.

This happens first:

bildschirmfoto 2018-03-07 um 14 22 47

As this is a direct access to the "to be cached" location it fails and causes a 404 page to be rendered which properly fetches the asset through the JSCombiner (and not directly):

bildschirmfoto 2018-03-07 um 14 23 07

@MorrisJobke
Copy link
Member

@juliushaertl I tested this by calling curl http://localhost:8000/index.php/js/core/merged-template-prepend.js\?v\=b2f9666b-0 directly ... don't know if this is supported, but I would think so, or is this only allowed to be called after the template is loaded (the template rendering causes the assets being compiled).

@juliusknorr
Copy link
Member Author

don't know if this is supported, but I would think so, or is this only allowed to be called after the template is loaded (the template rendering causes the assets being compiled).

Yes, the template needs to be loaded first, since that will iterate over included js/scss files and then run the combiner/scss cacher.

@juliusknorr
Copy link
Member Author

It would of course make sense to trigger that also for direct requests to the getJS route.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

It works if I open run the repair step and call http://localhost:8000/apps/files/ - it does not work if I open http://localhost:8000/js/core/merged-template-prepend.js directly. Fine for me for now.

@MorrisJobke
Copy link
Member

It would of course make sense to trigger that also for direct requests to the getJS route.

Let's do this in a followup PR.

@MorrisJobke MorrisJobke requested a review from skjnldsv March 7, 2018 13:34
@juliusknorr
Copy link
Member Author

Ok great. Let me adjust the tests then.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the fix-cache-prefixing branch from 7ef1012 to 573b940 Compare March 7, 2018 14:09
@juliusknorr
Copy link
Member Author

Done. Failing tests are unrelated.

@MorrisJobke
Copy link
Member

Done. Failing tests are unrelated.

Indeed - composer failure :/

@rullzer rullzer merged commit 7a7c350 into master Mar 8, 2018
@rullzer rullzer deleted the fix-cache-prefixing branch March 8, 2018 20:12
@juliusknorr
Copy link
Member Author

backport at #8745

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

Successfully merging this pull request may close these issues.

3 participants