[RFC][Discussion] Add session_gc() View as Markdown

Hi Andrey,

Hi Andrey,

On Fri, Apr 8, 2016 at 11:33 AM, Yasuo Ohgaki yohgaki@ohgaki.net
wrote:

Hi Andrey,

On Fri, Apr 8, 2016 at 4:57 PM, Andrey Andreev narf@devilix.net
wrote:

PS(gc_maxlifetime) is needed, so I fixed the last commit.

It is necessary for probability-triggered GC because you don't have
another
way of giving a TTL value then, but that's not the case for a direct
session_gc() call.

PS(gc_maxlifetime) is not related directly to probability based GC,
but it's about which session should be deleted.

Save handlers are supposed to delete inactive sessions exceeds
PS(gc_maxlifetime) when GC is issued. Save handlers are not suppose to
use PS(vars) directly and should use passed TTL parameter. Therefore,
the parameter is passed.

It is not directly related to gc_probability it is very obviously the
result
gc_probability-based design.

Still, even if we agree to disagree on that, it's not a blocker for
maxlifetime being overridable via a parameter.

We need TTL value and the TTL is gc_maxlifetime.
How would you specify session data to be removed?

TTL value is mandatory.

I wrote this pseudo-code in my very first reply to this thread ...

function session_gc($ttl = ini_get('session.gc_maxlifetime')) {}

If that's unclear, here's a more verbose way to put it:

function session_gc($ttl = null)
{
    if (empty($ttl))
    {
        $ttl = ini_get('session.gc_maxlifetime');
    }

    // ...
}

It just gives the ability to override the INI value for those who don't want
or need it.

I thought you claim that use of gc_maxlifetime is wrong.

3. If session_gc() bypasses the gc_probability, gc_divisor INI
   settings*, then why does it still rely on gc_maxlifetime?

Anyway, you're suggesting session_gc() should have $ttl =
ini_get('session.gc_maxlifetime') as the TTL parameter. This sounds
reasonable and I'll add optional 1st parameter.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

Hi!

Probability based GC is unreliable and can reactivate very old sessions
without "Precise session management RFC" and you opposed the RFC.

You know why I opposed the RFC - because it had huge number of things
packed together, some of which I agree with, some of which I disagree
and some of which I agree in principle, but would like to do in other
ways. I thought that the RFC would be much better to split up, and was
arguing so, but you decided it is better to proceed with everything as
one package, which is your right as RFC author, but it failed. So now it
is time to consider it piece by piece.

I think session_gc() would be a good addition, but I oppose the idea
that it should be the only method of GC. Many sites would have hard
time working with this method, either because of not having cron access
or not being able to configure it correctly, and would just resort to
implementing probabilistic GC anyway, introducing myriad of bugs on the
way. And the fact is, for most sites probabilistic GC works nicely. Only
sites that have very low traffic and very short session lifetimes would
have any issue with probabilistic GC.

If you have a site with 10 requests per second, and have GC with
probability of 1%, then it is functionally equivalent of doing manual GC
once per 10 seconds. Of course, you don't know when exactly, but a) cron
doesn't exactly guarantee precise timing either and b) I do not see why
it would be so important for a common application if the session is
cleaned up after 10 or 12 seconds.

There are other questions with regard to old sessions - but those are
not very relevant to GC model so I do not discuss them here. We can
discuss them separately.

Reactivating obsoleted/should be deleted session is wrong simply.
Why users shouldn't matter?

I never said "users shouldn't matter", so please do not ascribe it to
me. Of course users matter, that's why we are doing all this. The
question is how to help them.

I appreciate that you think manual GC is superior, and I fully support
inclusion of that option into PHP. What I however can not support is to
make this the only option, contrary to what worked in PHP for nearly two
decades. I want both your opinion and opinions different from yours to
be accommodated, and to that goal I propose to support both ways to do
GC and let the user choose which one is best for them.

Stas Malyshev
smalyshev@gmail.com

Hi,

Probability based GC is unreliable and can reactivate very old sessions
without "Precise session management RFC" and you opposed the RFC.

While I agree that calling session_gc() on a regular basis is the better
option, that doesn't mean probability-based GC is inherently unreliable.

The problem you're referring to here is a logical one, not caused by the
API, and is easily fixable - we just have to call SessionHandler::gc()
before the current session is initialized. That is, before
SessionHandler::read(), but after SessionHandler::open().
In fact, I think you've mentioned this before, so it's not like you don't
know about it.

Cheers,
Andrey.

Hi Stas,

Document calling session_gc() periodically is the best practice.

If you want to document usage of this new API as the best practice, it
would be unfair to the users if you don't also document the caveats that
come with it:

I also think it's wrong to document it so. While having periodical GC is
the best practice, probabilistic GC is calling GC periodically. True,
for some exceptional scenarios - like sites with very low traffic - the
period is unpredictable. And then it's recommended to explicitly call
session_gc(), and for these cases it is a great addition. But in a
typical medium or high traffic site the period is very predictable as as
such there's no way to document otherwise.

Problem of session GC is not only "when GC is performed" but also
"GC affects user experience" i.e. Occasional slow response.

Therefore, GC is better to be performed by non user process/thread. IMHO.
Exceptions are save handlers do not require GC at all. e.g. memcached

probability-based behavior (and how to do that; based on my
observations, the vast majority of users don't know how GC is triggered
at all)

And that's great BTW since majority of users don't need to know that -
it just works. We shouldn't ruin that.

Since we don't have precise session management (yet), users should
aware problems in probability based session GC triggered by user
requests.

Even if we have precise session management, large number of session
data will affect user experience with default session storage. i.e. files
save handler.

BTW, we already have feature that users must remove obsolete session
data files. If user uses multi level dir structure for files save handler,
files save handler will not remove obsolete session data at all.

PS_GC_FUNC(files)
{
PS_FILES_DATA;

/* we don't perform any cleanup, if dirdepth is larger than 0.
we return SUCCESS, since all cleanup should be handled by
an external entity (i.e. find -ctime x | xargs rm) */

if (data->dirdepth == 0) {
*nrdels = ps_files_cleanup_dir(data->basedir, maxlifetime TSRMLS_CC);
}

return SUCCESS;
}

• Trigger warnings when session_gc() is called while gc_probability is
  not 0.

This does not sound like a good idea to me. Why we should make it harder
for people to use it together? session_gc() has nothing to do with
gc_probability.

session_gc() will have this signature

int session_gc(void) // returns number of removed sessions

• To avoid re-activating expired sessions, trigger warnings when
  session_gc() is called after session_start()

Last time I've looked at the patch, session_gc() was required to be
called after session start. See:

https://github.com/php/php-src/pull/1852/files#diff-2e4264d70a35458a2241e27f76f4a93cR20

I think this is wrong (see my comments there) and in fact we should not
expose this complexity to the user - gc() should just do the GC and hide
the technical details.

I haven't update the patch according to discussion.
I'll update the patch soon.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net