Shutdown kernel for surrogates and handle as master request #34688

Toflar · 2019-11-28T13:56:24Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets
License	MIT
Doc PR

I feel like the current surrogate handling is incorrect.
The main issue here really is about surrogate requests. So let's think about a page structure that contains two ESI fragments like so:

/about-us
├── /_fragment?_controller=controller1......&_hash=supersecure1
└── /_fragment?_controller=controller2......&_hash=supersecure2

So let's see what HttpCache does:

Requests /about-us (real HTTP request, Symfony treats it as master request).
Requests /_fragment?_controller=controller1 (Internal surrogate request, Symfony treats it as sub request).
Requests /_fragment?_controller=controller2 (Internal surrogate request, Symfony treats it as sub request).

So we have only 1 PHP process that is started. Things might be shared across fragments.

Now let's think about what Varnish does:

Requests /about-us (real HTTP request, Symfony treats it as master request).
Requests /_fragment?_controller=controller1 (real HTTP request, Symfony treats it as master request).
Requests /_fragment?_controller=controller2 (real HTTP request, Symfony treats it as master request).

So we have 3 separate PHP processes, nothing can be shared!

Because of that, if we had no special handling for e.g. the request stack and also the session, the fragment requests would not be isolated. And especially for the session handling, this turns out to be pretty complicated. The SessionListener (or AbstractSessionListener to be specific) tries to keep track of every request and response, increasing and decreasing the stack etc.

However, apart from the fact that it is very complicated to understand, Symfony also behaves differently when running behind HttpCache or Varnish.
If we would reset the whole kernel for every surrogate request and actually execute them as master requests, it would behave the same as if Varnish requested the fragments (provided we reset and shutdown everything correctly).
This of course has effects on every listener that checks for if (!$event->isMasterRequest()) such as firewall listeners etc.

But wouldn't it be actually correct? Because that's exactly what happens when Symfony runs behind Varnish. Wouldn't we want to try and behave exactly the same, no matter what kind of reverse proxy is put in front of the app?

This is just a draft PR and again, no idea about the implications but I would like to get some feedback on this 😊

dbu · 2019-11-29T13:52:38Z

i was wondering why this is like it currently is, but never asked...
what are the upsides? performance?

indeed it would seem more consistent and robust to me to create a new kernel. in terms of practical issues, what does this solve? it forces people to be clean when using the build-in ESI support and will make switching to Varnish actually work without nasty surprises. additionally, some of the session handling can be simplified.
is the master/sub request thing used for other cases or only for this?

however, for the nasty surprise reason, i feel that this should be considered a BC break as the behaviour will change in a non-obvious way when you upgrade.

as the method is protected, we could also start recommending to adjust the kernel accordingly. or even only adjust it in debug mode and during test runs to detect problems, but keep the shared kernel in prod mode for better performance?

nicolas-grekas · 2019-11-29T15:49:32Z

src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php

+        // Shut down the kernel for every forwarded request so services etc. are fresh
+        $kernel = $this->getKernel();
+        if ($kernel instanceof KernelInterface) {
+            $kernel->shutdown();


I'm not sure this makes sense: Kernel::handle() calls Kernel::boot(), which then calls $this->container->get('services_resetter')->reset(), which is for that: resetting the container to a state that allows one request to not leak to the next one.
If there still is a leak, that's a bug with some services that aren't reset properly.
The proposed approach will kill performance.

I see. Well, the performance kill is not really an argument for me, it's the same if you use Varnish (actually even worse because of the PHP startup time). Anyway, if resetting the services is the way to go then I'm fine with that. Can you explain to me why we have a $sessionUsageStack in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php#L67 then? As far as I could tell this is to count the usages across multiple master requests which should never happen except for the HttpCache case? Does it? If not, then resetting the stack (which doesn't need to be a stack anymore then) would be more consistent with the rest of Symfony or am I wrong here?

AbstractSessionListener doesn't know anything about HttpCache - and shouldn't. So it has to stack to keep being agnostic, isn't it?
Should the services resetter be called between virtual ESI requests? I think so :)

Yes, I agree it shouldn't know about HttpCache. I don't know if it has to keep the stack to be agnostic. We could argue the same for every other service. Why is there the ResetInterface in the first place then? :)

Should the services resetter be called between virtual ESI requests? I think so :)

Yes, it should. That's why I proposed to actually shutdown and boot the whole kernel again. I know it's not performant but in fact that's exactly what happens when you use Symfony behind Varnish. You have a completely separate PHP process for every single ESI request. ESI has to be chosen wisely, that's the nature of ESI fragments 😊
But I'm also fine calling the services resetter, although in #34688 (comment) you said it's already done?

Don't trust me, trust the code :)
I'm not sure we get inside this "if", so maybe we should do a direct call to the resetter.

symfony/src/Symfony/Component/HttpKernel/Kernel.php

Line 101 in ae00ff4

if (!$this->requestStackSize && $this->resetServices) {

Shutting-down the kernel is no-go.

Sure, so here are my findings. I did test a simple site with one fragment, so this:

/about-us └── /_fragment?_controller=controller1......&_hash=supersecure1

Here's what's happening:

Kernel::handle() for /about-us

Kernel::boot() for /about-us. $this->requestStackSize is 0 and $this->resetServices is false. Note that after boot() $this->resetServices is set to true so the next time when we boot, it will be true.

AbstractSessionListener::onKernelResponse() detects a sessionUsageIndex of 10 in my case. Compared to end($this->sessionUsageIndex) which equals 0 it differs, so the response is turned into a private response correctly.

The ESI fragment is detected so HttpCache::forward() is called which calls Kernel::boot(). Because of $this->resetServices being set to true in step 2, services are now correctly reset and the service resetter is called. $this->resetServices is set to false so it's not done again.

We are now at Kernel::handle() again because that's called by HttpCache::forward() after calling Kernel::boot() in step 4. The services are not reset because $this->resetServices is now false but it's set to true again for the next handle() call.

AbstractSessionListener::onKernelResponse() detects a sessionUsageIndex of 16 now in my case. Compared to end($this->sessionUsageIndex) which equals 10 now, it still differs, so this response is turned into a private response as well.

So to sum up I think we can state the following things here:

This PR is not required because services are correctly reset and we don't need to reboot.

The sessionStack logic in the AbstractServiceListener could be implemented without the complexity it has now by using the ResetInterface like all the other services in Symfony for reasons of consistency. I even think, counting the usageIndex (which again brings a lot of complexity in my opinion) is not needed. The $session->hasBeenStarted() implementation would've been enough imho. I don't know why exactly this was implemented. Maybe there are other reasons that go beyond resetting between surrogate requests.

The $session->hasBeenStarted() implementation would've been enough

i have some very vagure recollections that in the beginning of symfony 2, there were problems where people accidentally started the session e.g. by just checking if there is anything, as that auto-created the session. at least at the time, the answer was to first check if a session exists at all. maybe this behaviour is a DX to help badly programmed applications that accidentally start a session to not break all caching? (though if that is the case, a phpdoc on the usageIndex to explain this would be a good idea)

does git blame or history somehow help figuring out when this was introduced?

Except supposed complexity, is there anything to fix then?
Otherwise, not sure we should care :)

maybe this behaviour is a DX to help badly programmed applications that accidentally start a session to not break all caching?

no that's not related, you still need to check Request::hasPreviousSession() for that.

I don't know why exactly this was implemented. Maybe there are other reasons that go beyond resetting between surrogate requests.

As far as I noticed, the counter is also increased if the security token storage is accessed, even if the session is not started.

Except supposed complexity, is there anything to fix then?
Otherwise, not sure we should care :)

Well there's so much complexity, we (@Toflar, me and other Contao) devs are banging our head at why this isn't working as expected and why it's so complex. Also, tracking the security token storage state inside a session is completely non-obvious and makes no sense except for it works that way.

I don't think there's an urgent fix needed here, but I'd love to see some DX improvements. Using the well-known/very clear kernel.reset to check the session usage is certainly one step in that direction.

Yeah we can close this because functionality-wise it's fine. Just DX is horrible tbh.

nicolas-grekas · 2019-12-19T15:53:47Z

Yeah we can close this because functionality-wise it's fine. Just DX is horrible tbh.

Thanks for the discussion, closing then.
About the DX, the session subsystem needs a serious update, this one is just a symptom.

Toflar · 2019-12-19T15:58:17Z

Thanks for the discussion too! I appreciate the fact that you're trying to understand our issues :)

Shutdown kernel for surrogates and handle as master request

b6bfcab

carsonbot added Status: Needs Review Bug labels Nov 28, 2019

Toflar mentioned this pull request Nov 28, 2019

[Security] Make stateful firewalls turn responses private only when needed #33663

Merged

Toflar force-pushed the shutdown-kernel-for-surrogates branch from becbd48 to 2db24b4 Compare November 29, 2019 11:03

HttpCache should shut down the kernel, not the surrogate

8d52622

Toflar force-pushed the shutdown-kernel-for-surrogates branch from 2db24b4 to 8d52622 Compare November 29, 2019 11:58

nicolas-grekas reviewed Nov 29, 2019

View reviewed changes

nicolas-grekas modified the milestones: 4.3, 3.4 Nov 30, 2019

aschempp mentioned this pull request Dec 3, 2019

ContaoFramework::setConstants() contao/contao#868

Closed

nicolas-grekas closed this Dec 19, 2019

Toflar deleted the shutdown-kernel-for-surrogates branch December 19, 2019 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Shutdown kernel for surrogates and handle as master request #34688

Shutdown kernel for surrogates and handle as master request #34688

Uh oh!

Toflar commented Nov 28, 2019

Uh oh!

dbu commented Nov 29, 2019

Uh oh!

nicolas-grekas Nov 29, 2019 •

edited

Loading

Uh oh!

Toflar Dec 2, 2019

Uh oh!

nicolas-grekas Dec 10, 2019 •

edited

Loading

Uh oh!

Toflar Dec 10, 2019

Uh oh!

nicolas-grekas Dec 10, 2019 •

edited

Loading

Uh oh!

Toflar Dec 16, 2019

Uh oh!

dbu Dec 16, 2019

Uh oh!

nicolas-grekas Dec 16, 2019

Uh oh!

aschempp Dec 16, 2019

Uh oh!

Toflar Dec 18, 2019

Uh oh!

nicolas-grekas commented Dec 19, 2019

Uh oh!

Toflar commented Dec 19, 2019

Uh oh!

Uh oh!

Uh oh!

Shutdown kernel for surrogates and handle as master request #34688

Shutdown kernel for surrogates and handle as master request #34688

Uh oh!

Conversation

Toflar commented Nov 28, 2019

Uh oh!

dbu commented Nov 29, 2019

Uh oh!

nicolas-grekas Nov 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar Dec 10, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Toflar Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dbu Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

aschempp Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

Toflar Dec 18, 2019

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Dec 19, 2019

Uh oh!

Toflar commented Dec 19, 2019

Uh oh!

Uh oh!

nicolas-grekas Nov 29, 2019 •

edited

Loading

nicolas-grekas Dec 10, 2019 •

edited

Loading

nicolas-grekas Dec 10, 2019 •

edited

Loading