[Session] Do not start the session unless it is (or has been) written to #6388

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet

TerjeBr commented Dec 16, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #6036
License of the code: MIT

The session->get (and the like functions) should only start the session in the current request if the session had already been started in a previous request.

@stof stof commented on an outdated diff Dec 16, 2012

...ttpFoundation/Session/Attribute/EmptyAttributeBag.php
+
+/**
+ * A wrapper class for an empty attribute bag
+ *
+ * @author Terje Bråten <terje@braten.be>
+ */
+class EmptyAttributeBag extends EmptyBag implements AttributeBagInterface, \IteratorAggregate, \Countable
+{
+ /**
+ * Constructor.
+ *
+ * @param EmptyStorageInterface $storage
+ * @param SessionBagInterface $realBag
+ */
+ public function __construct(EmptyStorageInterface $storage,
+ SessionBagInterface $realBag)
@stof

stof Dec 16, 2012

Member

Please keep the signature on one line

@stof stof commented on an outdated diff Dec 16, 2012

...Symfony/Component/HttpFoundation/Session/EmptyBag.php
+
+ /**
+ * The session bag this empty bag is a proxy for
+ *
+ * @var SessionBagInterface $realBag
+ */
+ protected $realBag;
+
+ /**
+ * Constructor.
+ *
+ * @param EmptyStorageInterface $storage
+ * @param SessionBagInterface $realBag
+ */
+ public function __construct(EmptyStorageInterface $storage,
+ SessionBagInterface $realBag)
@stof

stof Dec 16, 2012

Member

Please keep the signature on one line

@stof stof and 1 other commented on an outdated diff Dec 16, 2012

...Symfony/Component/HttpFoundation/Session/EmptyBag.php
+ * @var SessionBagInterface $realBag
+ */
+ protected $realBag;
+
+ /**
+ * Constructor.
+ *
+ * @param EmptyStorageInterface $storage
+ * @param SessionBagInterface $realBag
+ */
+ public function __construct(EmptyStorageInterface $storage,
+ SessionBagInterface $realBag)
+ {
+ $this->storage = $storage;
+ $this->realBag = $realBag;
+ $this->isEmpty = true;
@stof

stof Dec 16, 2012

Member

This is sueless as it already has a default value set in the property declaration

@TerjeBr

TerjeBr Dec 16, 2012

May be useless, but it does not hurt to be sure.

@stof stof commented on an outdated diff Dec 16, 2012

...ponent/HttpFoundation/Session/Flash/EmptyFlashBag.php
+ *
+ * @author Terje Bråten <terje@braten.be>
+ */
+class EmptyFlashBag extends EmptyBag implements FlashBagInterface, \IteratorAggregate, \Countable
+{
+ /**
+ * Constructor.
+ *
+ * @param EmptyStorageInterface $storage
+ * @param SessionBagInterface $realBag
+ */
+ public function __construct(EmptyStorageInterface $storage,
+ SessionBagInterface $realBag)
+ {
+ parent::__construct($storage, $realBag);
+ }
@stof

stof Dec 16, 2012

Member

Redefining the constructor doing nothing except calling the parent one is useless. Simply keep the inherited method

@stof stof commented on an outdated diff Dec 16, 2012

...onent/HttpFoundation/Session/Storage/EmptyStorage.php
+
+ if (!array_key_exists($name, $this->bags)) {
+ throw new \InvalidArgumentException(sprintf('The SessionBagInterface %s is not registered.', $name));
+ }
+
+ return EmptyBag::create($this, $this->bags[$name]);
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function getRealBag($name)
+ {
+ $this->isEmpty = false;
+
+ return $this->realStorage->getBag($name);
@stof

stof Dec 16, 2012

Member

Wrong indentation

Contributor

stloyd commented Jan 7, 2013

@Drak @fabpot Any points on those changes?

@ghost

ghost commented Jan 7, 2013

I am sorry but I'm -1 on this PR. I've procrastinated a lot about replying.

The PR is extremely convoluted with bags having to know about sessions and each bag having to be proxied. Bags themselves, should not be starting the session. It's full of code smell. It introduces a BC break by adding methods to an interface, which I am also not in favor of. (the PR summary text incorrectly say BC break "no").

I've not had enough time to really devote to looking at other possibilities but this is definitely not it, imo. Sorry.

TerjeBr commented Jan 8, 2013

Why do you take the principal stand that bags should not be starting the session?
It is the whole point of the code that if you do not have a session from a previous request, a session should not be started unless a bag writes something to the session. It means that the session start must be controlled in the bags, or this issue can not be solved.

I did not regard adding methods to an internal interface to be a BC break. But I guess you are right, it is technically a BC break, even though I think it will pass by unnoticed by most of the application programmers that is using symfony. So I have updated the description in the PR summary text.

TerjeBr commented Jan 8, 2013

@Drak, if you do not like the session to be started in the bags, may be you like better option 3 that I wrote on the original bug issue:

'3'. Leave the bags as they are, and delay the start of the session until just before the response is about to be sendt. Then a new response event listner has to be created that checks if anything has been written to any session bag, and start the session if that is the case.

I do not know if that will be any cleaner code, because then the session stuff has to have its own response event listener, and then the session code will not be independent from the rest of symfony.

Contributor

lsmith77 commented Feb 1, 2013

BTW this is quite an important feature for high load websites

Contributor

dlsniper commented Feb 4, 2013

@Drak could you help out making this a better PR?

It's the second time I've seen tickets like #6917 that rise this problem. And I think we can all agree with what @lsmith said about being a problem for sites with high load.

Do you think you could help us out with some ideas on how to do this, the proper way?

/cc @fabpot

@ghost

ghost commented Feb 4, 2013

I may not be able to answer well until the weekend or just after.

Good job @TerjeBr ! I'm glad to see this fix is going to make it into 2.3. Like @lsmith77 said, this is very important for high traffic websites.

TerjeBr commented May 12, 2013

Is it going to make it into 2.3? I have heard nothing from the main maintainers of Symfony about that. I am still waiting for feedback from @fabpot on either this issue or the parent issue #6036

Member

stof commented May 13, 2013

No it won't be in 2.3. New features cannot be added in it anymore as we already froze them.

Contributor

dlsniper commented May 13, 2013

@stof with all due respect this is something that appeared several times in the past and the PR is 5 months old. While it might be a new feature I could also consider this as a bug fix. Do you keep bug fixes out of 2.3 release? Imho this should have been added for 2.3 a long time ago...

@ghost

ghost commented May 13, 2013

@dlsniper better keep the emotion out of this. Fabien has already decided which PRs will be included in 2.3 and that decision is final so there is no point making noise about it.

Owner

fabpot commented May 13, 2013

This is an issue but we need to find the best solution to fix it. Unfortunately, we are not all in agreement with the right thing to do.

@ghost

ghost commented May 13, 2013

FYI, Fabien referenced this particular PR as a reference to what ticket #6036 talks about. There is no promise of anything being merged at all. Just that he'd look at it. He has since decided, regardless of whether you think this is a bug or a feature, that the issue will not be addressed until post 2.3

Post 2.3 means 2.4? Or 2.3.X? Ok guys, either way thanks for looking at it, keep up the good work.

For anyone else reading: Meanwhile just put all your authenticated routes under a prefix like "/user", that's what I do and it works.

Member

stof commented May 14, 2013

@disposable-ksa98 As this PR contains a BC break, it has absolutely no chance to be included in a 2.3.x bugfix release

TerjeBr commented May 16, 2013

It is only a BC break because a method has been added to the Symfony\Component\HttpFoundation\Session\Storage\SessionStorageInterface
I doubt if any application programmers using Symfony are using that interface in their own classes.

TerjeBr commented May 16, 2013

We should continue the discussion on issue #6036 to find a resolution to this. I have made my comments there, but no answer seems to be coming.

I'm not sure this was the case when I first reported the issue and tested it in 2.2.?... But now, in 2.3.1, the toolbar is creating a session too. So remember to disable it in config_dev.php when debugging this issue.

@ghost

ghost commented Jul 15, 2013

@disposable-ksa98 afaik the toolbar requires a session because it displays the session bags and metadata. If this is not the intended behaviour you should open a separate issue for it.

Member

stof commented Jul 15, 2013

The session metadata are collected only when the session is started: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php#L73

But is the session created while collecting the profiler data (so inside yout request) or when displaying the toolbar (in the ajax request loading it) ?

@stof I'm requesting "/", and it comes back with no cookies. But the ajax request to "/_wdt" comes back with the session cookie with path "/". Not sure if this helps.

@Drak I think it's the same issue, since what we are saying is that simply asking for session data shouldn't create a session if there wasn't one to begin with. I can't keep an eye on every component to make sure it's not creating a session. Better find a way to fix the session, or the problem will keep coming back.

Member

stof commented Jul 15, 2013

The issue is the code keeping flash messages for the auto-expiring bag: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/WebProfilerBundle/Controller/ProfilerController.php#L220-223

It should retrieve the bag only if the session is already started (same in other methods)

TerjeBr commented Jul 15, 2013

That is a good illustration of why this PR should be in Symfony 2. It would make the code in the Profiler controller just work, and the programmer that programmed the profiler would not have to worry about it. Now the profiler has to have added its own checks if the session was started in a previous request, before it can get the flash bag. My PR will do this check for all code in all applications once and for all (is that not what we have a framework for, to make life easier for application programmers?), and if a session had not been started in an earlier request/response return an empty flashbag and no session is started. But if anything has been written to the session bag, (and only then), then it triggers the start of the session.

TerjeBr commented Jul 26, 2013

I just wanted to correct @stof 's comment #6388 (comment)
He said: "It should retrieve the bag only if the session is already started (same in other methods)"
The correct thing is to test if the session was started in an earlier request/response. To just test if the session is already started will give you a wrong result if something has been written to the session earlier, but the session has not been started yet in this response.

TerjeBr commented Jul 26, 2013

@fabpot, what will be the fate of this PR. I have some ideas to improvements to my current code in this PR, but before I do any more work on it I would like to know if you plan to accept it into Symfony or not.

TerjeBr commented Oct 27, 2014

@fabot please comment on the fate of this PR. Would you like to see some further development on it?

Contributor

yguedidi commented Oct 29, 2014

ping fix @fabpot

mrclay commented May 31, 2015

I don't have enough Symfony knowledge to say whether it's worth making these particular changes, but some of the criticisms made here don't seem fair. All these "bags" are just abstractions, so the need to add connections between components isn't necessarily a sign of a bad fix; it might just be a sign that these particular abstractions have leaked.

Contributor

mcfedr commented Jul 27, 2016

The idea of this pull request seems very useful. I would agree with @mrclay comments. This is a DX issue, I dont think its worth hanging onto the idea of Bags being separate when it results in this bit of documentation

This means that if you need to avoid creating a session cookie for some users, it can be difficult

Very often, particularly high traffic, sites need to avoid creating sessions unnecessarily.

Member

HeahDude commented Nov 3, 2016

What should be done here? Is this still relevant today?

Member

javiereguiluz commented Nov 3, 2016

@HeahDude I haven't looked at this PR, but the fact that sessions are created automatically when you perform tasks such as checking if there are flash messages has always looked like a bug to me. Forcing our users to do checks like these ones to avoid starting the session unnecessarily looks like a hack.

Member

HeahDude commented Nov 3, 2016

@javiereguiluz Thanks for your response, I totally agree with you. But I was wondering if the work here is still a way to explore.

Contributor

mcfedr commented Nov 4, 2016

My 2p, its an issue that ideally would be fixed, but to me, this solution looks overly complicated.

Contributor

ureimers commented Dec 2, 2016 edited

Since four years our "solution" to this problem has been to use the NullSessionHandler on our production sites as otherwise, with one million page views per day, the session folders explode within a few days.
Hopefully we never need to add community and login support to those sites...

@nicolas-grekas nicolas-grekas modified the milestone: 2.7, 3.x Dec 6, 2016

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