Skip to content

Shutdown kernel for surrogates and handle as master request #34688

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

Closed
wants to merge 2 commits into from
Closed

Shutdown kernel for surrogates and handle as master request #34688

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6bfcab
Shutdown kernel for surrogates and handle as master request
Toflar Nov 28, 2019
8d52622
HttpCache should shut down the kernel, not the surrogate
Toflar Nov 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\HttpKernelInterface;
use Symfony\Component\HttpKernel\KernelInterface;
use Symfony\Component\HttpKernel\TerminableInterface;

/**
Expand Down Expand Up @@ -465,6 +466,12 @@ protected function fetch(Request $request, $catch = false)
*/
protected function forward(Request $request, $catch = false, Response $entry = null)
{
// Shut down the kernel for every forwarded request so services etc. are fresh
$kernel = $this->getKernel();
if ($kernel instanceof KernelInterface) {
$kernel->shutdown();
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@nicolas-grekas nicolas-grekas Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@nicolas-grekas nicolas-grekas Dec 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. Kernel::handle() for /about-us
  2. 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.
  3. 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.
  4. 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.
  5. 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.
  6. 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:

  1. This PR is not required because services are correctly reset and we don't need to reboot.
  2. 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

if ($this->surrogate) {
$this->surrogate->addSurrogateCapability($request);
}
Expand Down