Add @znerol to commit credit
Problem/Motivation
Twig 1.22.0 introduces the concept of cache classes so we don't have to override as much of \Twig_Environment, but it also breaks our overrides.
Proposed resolution
Upgrade Twig to 1.22.0 and introduce our own cache class in this issue. Twig 2.x compatibility will be handled in #2568181: [META] Update to Twig 2.x in Drupal 9.
Remaining tasks
Review, address feedback, commit.
User interface changes
n/a
API changes
There will be some changes to \Drupal\Core\Template\TwigEnvironment which hopefully nobody is relying on but we've seen in contrib people extending that instead of the vendor \Twig_Environment for no good reason. In general those shouldn't break, people would have to be relying on methods we will be removing around caching, storage, etc.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#79 | interdiff.txt | 850 bytes | star-szr |
#79 | upgrade_to_twig_1_22-2568171-79.patch | 59.65 KB | star-szr |
#76 | upgrade_to_twig_1_22-2568171-76.patch | 59.66 KB | cilefen |
#76 | interdiff-2568171.txt | 2.31 KB | cilefen |
#73 | interdiff.txt | 798 bytes | star-szr |
Comments
Comment #2
star-szrComment #3
star-szrTechnically this one doesn't have to happen here, I may move this into the general Twig 2.0 compatibility issue which I'm creating shortly.
Comment #4
star-szrThis removes that change so it doesn't conflict with #2568181: [META] Update to Twig 2.x in Drupal 9. Or at least doesn't conflict as much…
Comment #5
star-szrBased on my testing so far we don't need to do any special handling for the race condition anymore so this can potentially be removed but needs some more confirmation and testing. #2429659: Race conditions in the twig template cache
Comment #6
dawehnerGreat work!
Afaik we don't need this either anymore.
IMHO pointless as its just the BC layer we we no longer have to care about.
Just an idea, we could write a NestedTwigCache which gets the PHP and the NULL one injected?
Let's rename it to "cache"
ah nice!
I think we would maybe better have a workaround, as one process might cleanup the files, while another has had made the has() call already.
What should we do in that case? Are we fine with the cache to disappear at some point in time?
Comment #7
star-szrYeah I thought it would be cool if we could kind of do a chained cache similar to how we do Twig loaders using the vendor \Twig_Loader_Chain. Now I have a better idea of how it would actually be implemented after having some distance from all the other changes. Thanks for the review, if nobody beats me to it I'll work on this again tonight :)
Comment #8
webchick#2568085: Lock Twig on ~1.21.2 until we are able to upgrade to 1.22.0 has been committed.
Comment #9
star-szrThanks @webchick here's a tiny reroll so this still applies, no other changes yet.
Comment #10
star-szrComment #11
star-szrPicking this up again.
Comment #12
star-szrAddressing feedback from #6, I don't know the answer to #6.7 so just put some code in for now.
This brings in the concept of a chained cache class. Overall I'm not sure how much further this gets us unfortunately. I tested the eight combinations of returning FALSE or throwing exceptions in TwigPhpStorageCache. I tested each scenario with warm and cold Twig caches and found no differences.
It falls down if load() in the TwigPhpStorageCache class fails because when it gets to the Null cache class there is of course nothing to load. So this seems to be the race condition again and I don't know any way we can fix this in Twig cache class land because load() only has the cache key so it can't compile the template source as far as I can tell unless we reverse engineer the cache key and inject the Twig environment or something similarly absurd.
So maybe we still are stuck with overriding loadTemplate() for ourselves…
Feedback, please! And let's see what the testbots say.
Edit: Actually I think I changed something partway through testing because now simply returning FALSE from has() is resulting in a class not found, updated the table accordingly after retesting. A bit whack-a-mole.
Comment #13
star-szrSo removing the $cache->has() from \Drupal\Core\Template\TwigCacheChain::load() fixes that case (returning FALSE from has() in TwigPhpStorageCache) but results in a WSOD when returning FALSE from has() and throwing an exception in the cache writing but only when the Twig cache is warm.
Fun stuff.
Comment #14
hussainwebI am wondering why we are doing this here. I am uploading a patch with changes in
core/lib
from #12 and Twig 1.20.0. If this patch passes (as #12 is passing), that means we can safely go ahead with this version constraint. Otherwise, we should raise the constraint to~1.22
.For reference, this is the command I used to load twig 1.20.
Comment #15
dawehnerSigh, ... because twig 1.22 introduced some additional APIs we want to implement to be able to switch to twig 2, see issue summary.
@cottser
I'll give feedback later. Great research!
Comment #17
hussainweb@dawehner: I think one of us is misunderstanding. I am expecting the patch in #14 to fail as the API changes were described as BC breaking somewhere. If the tests pass, then all is fine. Otherwise, we should raise it to
~1.22
.Comment #19
hussainwebAs expected, the patch in #14 failed. I have updated the version constraint to
~1.22
. The interdiff is from that patch. Also, it seems there was a new version1.22.1
in the meantime, which is included in the patch.Comment #20
hussainwebComment #21
dawehner@hussainweb
Thank you for moving the issue back into scope :)
Comment #22
hussainweb@dawehner, :)
I think it was still in scope. The line I was picking about was in the patch and important. It said that the changes could work with Twig 1.20, which was certainly not the case. Anyway, it's now set at ~1.22 as originally intended.
Comment #23
dawehnerJust some minor improvements. I think this is looking pretty good. It would be great if fabian could have a look at it, fabien of course too.
Comment #26
star-szr@dawehner thanks! Those changes look great. I was wondering if there was some nice assertion thingy to use :)
I'm wondering if this fixes the problems I saw, I can test later. Would this result in always
eval()
'ing the template?Comment #27
star-szrI also wonder if other than the assertion this might be of interest to upstream since there is already a Twig loader chain.
Comment #28
webchickDue to the BC break, I think this needs to be critical.
Comment #29
star-szrNot sure about the criticalness. To me all this may be considered internal but not sure.
I assume this will work better for the assertions.
Comment #31
dawehnerMh, I think its a BC break for internal APIs not for themes, so I'm not convinced that this needs to be done now.
On the other hand we want to follow twig 1.x at least in order to be able to catch security issues.
Ups, this was a c&p problem :)
Comment #32
star-szrJust a heads up, always trying to write the cache seems to result in a WSOD with cold caches.
Trying this assert business again. \\ \ \ \\ \ \ The docs seem to be quite wrong on this.
Edit: Interdiff is from #23.
Comment #34
chx CreditAttribution: chx commentedI filed #2569049: Add a hook_requirements() that warns if assertions are turned on and discourage/remove double quotes in assert()
Comment #37
dawehnerIf we assertion is the problem, just remove it. I was just adding as I considered it to be a better solution compared to the if instanceof calls. Do we actually pass something in which does not implement the interface?
Comment #38
dawehnerWe should certainly explain why we need both here: race conditions when some process clears it + general fallback
Comment #39
star-szrAgreed we need a comment there. I don't think it's the assertions causing problems now. When there is a cold cache if you don't return after the phpstorage write you get a WSOD.
FWIW those if's were modeled after the twig loader chain class.
Comment #40
star-szrI'm not sure always trying to write the cache helps us, at least not in the scenario @dawehner brought up in #6.6:
Here's the code from the vendor \Twig_Environment with the BC handling removed (we're not using the BC layer):
So this just adds back the return because things seems to be failing at least with how the vendor Null cache is implemented (it's just an
eval()
).Comment #41
star-szrI forgot to add a comment explaining why we are adding the null cache class to the chain, so needs work for that at least and I'm done for the night.
Comment #42
dawehner@Cottser Ah yeah, well I think this is a bug, see https://github.com/twigphp/Twig/issues/1832 ... I mean basically for now we never get to the null callback, as we always write to just the phpstorage based one.
Assigning to fabian for a review, as proper subsystem maintainer.
Comment #43
Wim LeersIt's not just the cache classes to chain. It's an ordered list of cache classes to chain. Right now, the comment indicates that the order doesn't matter, but it of course does matter greatly.
This is very strange, at least needs a comment.
Nit: s/twig/Twig/
Nit: Everywhere else in this class, we say
mtime
, so shouldn't we do here too?Comment #44
dawehnerLet's at least temporarily try it out to see why its problematic, so we can document it properly
Comment #47
star-szr@dawehner thanks for that upstream issue, and thanks @fabpot for the quick turnaround there.
The try/catch is so that the cache class can notify the cache chain to go to the next cache on failure or some other condition. I used RuntimeException because the Filesystem vendor cache class already uses that.
Here is a patch that subclasses the new vendor Null cache class' has() method to check if the buffer contains the eval()'d template. Based on our chained loader's implementation of load() it will never get to the vendor's Null cache class load() because we check has() first, and the Null cache class' has() method always returns false. But I'm pretty sure we do want to check has() first.
I don't have time to thoroughly test this now but we'll see what the bots say about this theory.
Edit: Just a note that I think #44's interdiff (and patch) is from #23, and this interdiff is from #40 with #44's interdiff applied :)
Comment #48
Wim LeersThat looks much better :)
Comment #50
Wim Leershttps://github.com/twigphp/Twig/issues/1832 causes Twig's NULL cache to now return the class name for
generateKey()
(see https://github.com/twigphp/Twig/commit/9fde02fe552256722466c9985520b6f50...), and so… this assertion no longer is true:AFAICT we should replace it with a
TwigEnvironment::getCache()
assertion. This patch does that.Comment #51
star-szrThanks @Wim Leers!
Also
private
messes things up so this brings some more things over from the vendor Null class. If there's a better way to do this I'm all ears. But I tested by just removing the PhpStorage cache class from the chain and it didn't work at all of course because it wasn't able to check the private buffer property.Comment #53
dawehnerDoes this then actually works as a fallback? Let's assume has() return TRUE, and then in the meantime something clears the stored templates ...
Comment #54
star-szrI think it would be good to again test what happens when parts of the PhpStorageCache fail as I did in #12 but that doesn't really handle the race condition scenario, just makes sure we have fallbacks.
I still think the scenario you described would result in class not found because from loadTemplate() as soon as has() is TRUE then no writing happens, it goes right to load. So once you get to that point if the PhpStorage suddenly goes away then there would be nothing to load from the null cache as nothing has been written.
@dawehner I am assuming in #53 you're still talking about clearing the PhpStorage cache storage, not the Null cache storage.
@joelpittet on IRC mentioned locking, I have no idea if that's something we can incorporate here or not.
Comment #55
dawehnerMh, how would that help? We aren't in the process of writing, but rather on loading. Just the files disappear from disk, we should provide some fallback for that.
Comment #56
dawehnerThe easiest thing we could do is to simply remove the chain and put the fallback logic into load of the PHPstorage cache. This feels like a good fix for the problem.
@Cottser @Fabianx Any thoughts about that?
Comment #57
star-szrDiscussed in IRC, we have a plan. Working on it.
Comment #58
star-szrRight now I think we still need to override loadTemplate() but putting this up as kind of progress. It should fail some tests.
Comment #59
dawehnerJust as idea, we could actually provide some testing, by mocking the way how storage and return has() TRUE but then let load() fail. In that particular case though, write() would not be triggered and this protection would never help. Mh, this is tricky.
Can we check whether we have $this->templateSourceCache available? Otherwise we can really not do anything beside failing.
Comment #62
star-szrCommented upstream because I think we are stuck: https://github.com/twigphp/Twig/issues/1811#issuecomment-140833294
Comment #63
star-szrFeedback on this approach please (humans and testbots).
This would need a PR upstream but want to see what people here think before sending it.
Comment #64
dawehnerThis looks pretty solid for me. I'd though add a comment in the Twig_Environment class explaining why we need such a call (race conditions ... )
Comment #65
Fabianx CreditAttribution: Fabianx as a volunteer commentedSo - I read this quickly when it was assigned to me and it looked good.
The biggest problem we faced in the race condition is that the template _could_ be loaded, but then the class was not there.
The problem with the approach here is that we don't have the class name available, so we can't do the if !(class_exists($class, FALSE) dance ...
That makes this a little more problematic, than I would like to and I agree that we likely need to fix this in PHPStorage itself.
Comment #66
znerol CreditAttribution: znerol commentedWhat exactly is the root of this problem? Storage
load()
must not returnTRUE
unless the file has been successfully included. I frankly do not expect that this is an issue with the current implementation of php storage.On the other hand the upstream code clearly contains a race condition and we used to work around it by overriding
loadTemplate()
. In my opinion we should file an upstream issue and stick to the override for now.Comment #67
star-szrYes, just spoke with @znerol at the extended sprints in Barcelona. The critical is just make this work for Twig 1.x, and making our stuff 2.x compatible would be a major: #2568181: [META] Update to Twig 2.x in Drupal 9
Working on a new patch now.
Comment #68
znerol CreditAttribution: znerol commentedFiled: https://github.com/twigphp/Twig/issues/1836
Comment #69
star-szrSomething along these lines.
Interdiff is from #58 because I reverted all of #63.
Comment #72
star-szrI think the new Null cache class (upstream) in both 1.22 and 1.x-dev doesn't protect against redeclaring classes. May need to add our own null cache for the time being.
Comment #73
star-szrThanks to @znerol for the pair programming, found there is an extra layer here that is no longer needed.
Historically (all the way back to #1696786: Integrate Twig into core: Implementation issue) before there was such thing as cache classes in Twig we didn't have has() so we did this inside our loadTemplate() method:
Now having this extra load/write/load is just causing problems because when caching is disabled (therefore the upstream Null cache class is being used):
Therefore with the patch in #69 when Twig cache is disabled (such as during install) the Null cache tries to write the same class multiple times even if the template is only called once.
Attached removes the now unnecessary hunk. If a race condition occurs we compile and eval. Really the only changes from the upstream loadTemplate are:
Comment #74
znerol CreditAttribution: znerol commentedLooks like dead code. It seems that `load()` ignores `templateSourceCache`.
I realize that this comment is from the old code, it was a bit unclear already there. However, now it is technically incorrect. We do not override anything here.
Maybe just delete the comment here and instead add a description to the class which explains the reason for the twig extension hash.
Perhaps something like this:
The `Twig_CacheInterface` does not specify a return value and the twig implementations never return anything, so we shouldn't either.
Comment #75
star-szrThanks! Will work on those tomorrow.
Comment #76
cilefen CreditAttribution: cilefen commentedthis is the suggestions from #74.
Comment #77
star-szrAwesome thanks @cilefen!
Comment #78
dawehnerIts not really optional, isn't it? The NULL indicates that somehow.
Let's typehint the interface consistently, I'd say.
Just curious whether we think this is the right thing. In case the cache entry is gone, do we maybe better want to use 0, so things are invalidated?
Comment #79
star-szrI think 0 does make more sense if we don't know the state of the cache. This addresses #78.
Comment #80
cpj CreditAttribution: cpj commentedI've reviewed the changes, which make sense and look fine to me. I've also done some local testing, including enabling / disabling parts of the new TwigPhpStorageCache, and it seems to behave as expected. Therefore I'm marking it as RTBC, but perhaps others also want to test ?
Comment #81
znerol CreditAttribution: znerol commentedIt seems to me that 1.22.0 now takes into account the loaded extensions when computing the class hash (see also PR 1824). So I think we might ditch
twig_extension_hash
and the compiler pass. Maybe in a follow-up?Comment #82
star-szrThank you @cpj, assigning to @Fabianx because I'd like him to have a look before we commit.
I created a follow-up to look at removing TwigExtensionPass: #2571613: Consider removing TwigExtensionPass in favor of Twig's newly built-in handling of changed extensions
This issue and patch is just so we're safe to upgrade the 1.x path, and 2.x compatibility will be handled in #2568181: [META] Update to Twig 2.x in Drupal 9. Updated the issue summary with that as well.
Comment #83
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC + 1 - This looks much nicer as an overall solution and also has less invasive changes.
Comment #84
alexpottCommitted 8b60468 and pushed to 8.0.x. Thanks!
Comment #86
star-szrYay!
Relevant: #2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now