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

[HttpKernel] fix session tracking in surrogate master requests #27467

Merged
merged 1 commit into from Jun 19, 2018

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Member

nicolas-grekas commented Jun 1, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Spotted while looking at ESI fragments resolved byHttpCache: right now when the master request starts the session, fragments are not cacheable anymore, even when they do not use the session.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 6, 2018

(failure will be fixed once merged up to master)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 14, 2018

ping @symfony/deciders votes pending, this "just" fixes ESI with HttpCache, which are broken (found in training sessions by @jpauli )

@stof

stof approved these changes Jun 18, 2018

Looks good to me

@nicolas-grekas nicolas-grekas merged commit 146e01c into symfony:3.4 Jun 19, 2018

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 19, 2018

bug #27467 [HttpKernel] fix session tracking in surrogate master requ…
…ests (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] fix session tracking in surrogate master requests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Spotted while looking at ESI fragments resolved by`HttpCache`: right now when the master request starts the session, fragments are not cacheable anymore, even when they do not use the session.

Commits
-------

146e01c [HttpKernel] fix session tracking in surrogate master requests

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:session-count-usage branch Jun 20, 2018

@stefandoorn

This comment has been minimized.

stefandoorn commented Jun 22, 2018

@nicolas-grekas Maybe unrelated, maybe not, I just upgraded from 3.4.2 to 3.4.11 on a site that heavily uses ESI fragments, and performance degraded within a second. Had to revert the upgrade (didn't expect this - locally no issue) to make performance be 'normal' again. Unfortunately no benchmarks or proper debug information available - locally also Blackfire doesnt show me any real differences.

Might be related to this?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 22, 2018

Only a profile could tell...

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 22, 2018

Maybe try branch 3.4-dev?

@stefandoorn

This comment has been minimized.

stefandoorn commented Jun 22, 2018

Yeah, was afraid about that :-D Will see if I can get that :) And I'll try the dev branch, good one.

@stefandoorn

This comment has been minimized.

stefandoorn commented Jun 22, 2018

I can confirm that on 3.4.2 files were being written to the http_cache folder inside the cache folder, but on 3.4.11 it's not happening. Tested locally.

@danrot

This comment has been minimized.

danrot commented Jun 26, 2018

@nicolas-grekas Are you aware that the session is always used in tests now? That's because of this $session->isEmpty statement. And I am relying on that in one of our tests. For now I had no better way to deactivate the session completely, which also doesn't feel very good...

Isn't that something that should be fixed in Symfony? I don't really see a nice way of handling this in our codebase (there are a few ways, but I would consider all of them dirty).

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 26, 2018

@danrot I know about #27714

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 27, 2018

@danrot can you please check #27714 and confirm it fixes the issue for you also?

@danrot

This comment has been minimized.

danrot commented Jun 29, 2018

Just tested it, it works 👍 Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment