[PHP-INTERNALS] Revert session_serializer_name(), session_gc()

Hi Andrey,
Users want to have contorl *when* gc is performed rather than luck. This is
reasonable. IMO.

There is session_module_name(), why not session_serializer_name()?
Many 3rd party save handler implements many serializer.

session_gc() is nothing to do with performance. Users may abuse. The same
argument applies to gc probability.

  I'd strongly suggest that these 2 functions should be removed before
These are simple additional APIs.
If your argument applies, there should be number of new RFCs.

Regards,

Hi Yasuo,
I like control, it's nice and *I already have it*:

     ini_set('session.gc_probability', 1);
     ini_set('session.gc_divisor', 1);
Why not session_cookie_lifetime()?
Why not session_cache_limiter()?
Why not session_entropy_file()?
etc.

"Why not" is never a good argument in programming and really, when is
the last time that you've seen something accepted here on internals
because of "why not"?
I'd rather question if session_module_name() is useful, or any
function that's an alias for ini_set('something', ...).
The problem is, users WILL abuse and that's bad.
Think of what the manual page would say about that function. Something
in the lines of:

     Calls the session garbage collector, ignoring
session.gc_probablity and session.gc_divisor.

What does that say to an inexperienced and even to the intermediate
user, who doesn't even know what GC is?

     "Hey, this collects garbage! It's nice to clean things up, I
should always call it!"

Yet another bad practice spreads.
Sure, that *could* apply to gc_probability, gc_divisor, but those are
*necessary* options and they are not easy to find unless you are
specifically looking for them.
Actually, if my argument applies - both feature requests should've
been ignored. Lots of people make "this would be nice" feature
requests without having the knowledge and experience to know if it
would really be "nice". That's why there's a bunch of smart people
here to take the decisions for them, collectively. I myself have been
forced to write an RFC for a smaller and less impactful change, but
that's not the point. Both of these functions should've at least been
discussed here on internals.

And finally, do you assume that there's demand for these features?
session_gc() specifically was requested 13 YEARS ago and it didn't
even get a "+1" comment in that timeframe. There are people on this
list who'd question user demand even if people are tweeting about the
feature with fingers crossed that it would be implemented ... these
are two that hardly anybody cares about.

Cheers,
Andrey.

Hi Andrey,

Of course you can.
It does not look nice as API. API should have good name for it. IMO.

session_gc() is useful for both low traffic site and high traffic site.
I don't see reason not to have explicit gc API. User may simply call

session_gc();

periodically by cron, etc.


Those aren't changed by programs often.

As I wrote, 3rd party save handler implements their own serializers.
Not like above example, it is likely called with session_module_name(). e.g.

session_module_name('memcache');
session_serializer_name('igbinary');

It looks better than

session_module_name('memcached');
ini_set('session.serializer_handler', 'igbinary');

It's reasonable to have session_serializer_name(), since it would be used
with session_module_name(). API should have good name here, too.

We may discuss what we should do with module functions that modify it's
INI. There are number of them already. I think you need RFC for this.

My opinion is "There should be API with obvious name, if it is used often
or mandatory".

Regards,

Hi,
There's a very good reason not to have it, you just ignored most of my
previous email which explained exactly that.
Oh, trust me - cookie lifetime is changed way more often than the serializer.

And how do you define often anyway? Sessions are something that are
setup once, it's rarely the case that a programmer needs to change any
of the session INI settings more than once in the same program.
It's your opinion that this is "likely" done that way. I for one have
*never* used session_module_name().
And again, this is not something that people would write more often
than once in the whole application.
It doesn't look better than:

ini_set('session.save_handler', 'memcached');
ini_set('session.serialize_handler', 'igbinary');

IMO, this is more explicit and more understandable for people who
haven't read the manuals for the "custom" functions.
I'm again questioning whether session_module_name() is
useful/necessary in the first place.
On this - not. ;)


Cheers,
Andrey.

Hi Andrey,
Users may abuse INI or API.
If you worry about abuse, it would be better to have API and warn users.

Users should purge deleted session data for security reasons. It is
especially important for low traffic sites. High traffic sites may need
periodic purge due
varying request rate. Not having GC API is API design bug.

If there should not be convenient API, file_get/put_contents() aren't
needed neither.
It's bad practice, indeed.
User should not change default 0 (session cookie - i.e. cookie only lives
in browser's memory and destroyed by browser termination)

If users need auto login or like, they should implement it properly by
using setcookie(). (i.e. Never use session ID cookie for long life session)

And how do you define often anyway? Sessions are something that are
It does not matter much if you use often or not.
As an API design, not having session_serializer_name() is strange.

I don't mind having discussion deprecating all API that modifying its INI.
I'm some where between +1 and 0 for this.

IMO, this is more explicit and more understandable for people who
Consistency matters. I'm trying to be consistent with current way.

I'm tends to agree deprecating all INI modifying APIs ;)
Let's have the RFC, I'll vote +1 for it.

Regards,

Hi again,
I don't think you understand what my point is ... the API itself must
be hard to abuse.
The INI settings are not that often misused because they are hard to
find for anybody that's not specifically looking for them. This is not
the case for functions and exposing the GC as a function is something
that would at least triple the abuse.
Nonsense. This is not even worth explaining why.
I was just giving you an example of how your assumption is wrong,
auto-login is not the subject of this discussion.
You just changed your argumentation from "it's often needed" to "it
doesn't matter if it's often used, but not having it is strange".
Again, my stance is that the existance session_module_name() is
strange, not the other way around.
So ... you'd vote +1 for deprecating all similar functions, yet you
just added another one without even discussing it? :)

That would be a nice RFC indeed, but I still stand behind my initial
suggestion - both of these functions should be excluded from the 5.6
release.

Cheers,
Andrey.

Hi Andrey,

The root cause is design bug of GC.
We may fix this with other method. This would be the best solution.
It's possible that save handlers use session.expire setting to execute GC
periodically.

This is the common abuse of session. IMO.
"often or mandatory"
I think it's both.


No problem.

It would be easier to understand if you explain your point ;)
I'll fix root cause of GC issue later. GC should not depend on luck.
Are you going to write RFC for INI issue?

Regards,

Hi Yasuo,
Not at this time, it won't make it into 5.6 anyway.
Julien Pauli already suggested that we should discuss that for PHP6
... I have some ideas, I'll start a discussion about them soon. :)

Cheers,
Andrey.

Hi Andrey,

Save handler may keep & check last GC time. If it's over session.expire,
perform GC.

Issue is that "session data that should be deleted" is not deleted and
could be alive long term due to current design. Current design is good
enough for many usage, but it is not the best practice obviously.

"Keep & check last GC time" requires additional overhead for every request.
This is the reason why I prefer to have session_gc() and encourage users to
use it properly.

Regards,

That would have a "scoping" problem: session.expire, at least in theory,
can be different for different scripts / usages of session in the same
application, but such a global last-GC timestamp would be... global.

Also generally GC is a costly global operation, slowing down the request
that hits it (probably that's why you envision a cronjob right?)
The _proper_ solution, would be to keep a timestamp stored with each
session. kind of a last-modified timestamp. (expiry does work off last
modification, right?). Anyhow, the trick is to check at session_start
whether that timestamp plus whatever session.expire is set at, already is
in the past - and when that happens, destroy the one session on storage and
act as if it hadn't been found.

This way there is no longer any correctness problem, and a GC is only
needed to clean up sessions that are never requested again - and for that I
fully agree that a separate - e.g. cron driven - "GC now" job would be best
(not affecting latency of running frontent requests too much).

Of course, going to such a solution, requires a change to all session
implementations in that they must provide for storage and return of such a
last-modified timestamp (or othewise the improvement cannot be realized for
a non-updated-to-the-new-scheme session handler)

best regards
   Patrick

Hi Patrick,
Yes. This is issue for save handlers that should perform costly GC.
I agree. It's proper and feasible solution. I've sent issues about
session_regenerate_id() in other thread. It could be solved by time stamp
stored as session data also. Storing time stamp out side of $_SESSION data
would be cleanest solution, but it requires new interface.

It may be good idea to write a new save handler interface while keeping
older one.

Regards,

It would really helpful to target a later versions, maybe 6 and see
what we can do while minimizing the impact in userlands (whether or
not custom session handlers are implemented).

But all the discussions and RFCs about basically the same needs have
been very confusing.

What do you think? Is it something you could consider?

Hi,
Such a timestamp already exists, you can't implement sessions without
one because there would be no way to know when a session have expired.
For the files handler, it's filemtime() (afaik) and the behavior that
you described is probably already in use (I see no reason not to).

However, that's not in the scope of GC.
I am deffinately a proponent of that. I already mentioned earlier that
I have some ideas ... this is one of them.

However, while Yasuo seems to always be keen on creating RFCs
immediately, I'd rather focus on the existing ones that are session
related:

https://wiki.php.net/rfc/secure-session-options-by-default
https://wiki.php.net/rfc/session_regenerate_id

5.6 is really close and this is important stuff that was supposed to
go in, but has not had updates recently and lagged behind.
Changes for future versions can wait a few days/weeks ...

Cheers,
Andrey.

Hi again,

Another 2 items that didn't go through the RFC process or any discussion:

  - Implemented Request #20421 (session_abort() and session_reset()
function). (Yasuo)

https://bugs.php.net/bug.php?id=20421

Does this change mtime of the session?
I also have other (rather philosophical) arguments against this, but I
don't want to get deeper into it now ... it should just be reverted
together with session_serializer_name(), session_gc() on the basis of
no discussion at all.

  - Implemented Request #17860 (Session write short circuit). (Yasuo)

    https://bugs.php.net/bug.php?id=17860

The problem with that is that there's no API exposed for custom save
handlers to decide when to only update mtime or the whole session. I
think the question was exactly about that, actually.
It is also not clear how is that implemented and for which of the
available session handlers ... is it all of them?

Then later (note that it *already is implemented*), it was voted as
part of this RFC, which made it optional:

     https://wiki.php.net/rfc/session-lock-ini

This RFC is broken to begin with ... what ended up as accepted from
this RFC was actually very far from the main intention behind it (and
it silently accepted passing ini options to session_start() with no
explicit voting for that - just saying, otherwise I like that
feature).
Also, there's no description for "read_only" except the obvious - that
it only reads. It should at least mention that a shared lock can be
used for that case.

IMO, if this feature had gone through proper discussion instead of
just presenting a yes/no option as part of another change, we'd end up
with a different solution.

Actually, I'd like to ask if it's possible for an RFC to override a
decision taken from a previous one? And can it be done quick? Or if
not - can that be postponed for 5.7?
The lazy_write option is nice, but it's not optimal and it would be
embarassing if it makes it into 5.6 only to be later reversed in favor
of a better solution. What I'm thinking of is a session_is_changed()
function and "lazy_write" at all times (there's no reason for that to
be optional).

Regards,
Andrey.

Hi Andrey,
These are covered RFC discussions and/or some other discussions in this
list.

abort and reset were discussed in RFC discussion. It was't a long
discussion though. Since this is just a simple API addition, it added
simply.

What's your reason?

This one is discussed in following RFC, since it affects existing behavior.
Some one pointed out and the RFC was created.

Then later (note that it *already is implemented*), it was voted as
You should comment during discussion. There was long discussion for this.

Shared lock could be used easily for some session storage, but it requires
save handlers modification and it could be implemented _independent_ of
read_only. Web apps must return response ASAP. Therefore, any locks that
could be avoided should be able to be avoided including read(shared) lock.

Anyway, read_lock is confusing unless save handler API supports it. i.e.
User cannot know if it really support read_lock like current
use_strict_mode.

Why not write a RFC for read_lock? I'm OK to implement it if you would like.

Regards,

http://markmail.org/message/om3ofpjlit7iq6d2

Is this the discussion you're talking about? If so, session_abort()
and session_reset() are barely mentioned in it, and no wonder - it was
during the holidays.
That's not a productive discussion ...
The RFC mentions a 'lazy_write' option and voting ended at the end of February.
The feature (not optional) was committed way earlier.
Sorry, I wasn't around during that discussion.
That's why I'm asking if a new RFC can override the old one.
I'm talking about handlers that PHP itself ships, they all implement
locking (exclusive locking).

An exclusive lock can only be obtained by one process and all others
will have to wait for it to be freed.
A shared lock can be acquired by multiple processes and as such, is
concurrency-friendly. However, it is only usable if sessions are in
read_only mode, hence why I'm suggesting it.
Users needs to know if they are in read_only mode.
Currently they can't, because the RFC proposed it an obscure setting
available only to session_start(). If it was an INI setting, it
could've been readable.
Because this is one of the few session-related things that doesn't
need an RFC, but rather common sense.
It's just an implementation detail that was not described in the RFC.

----------

All I'm saying is, this whole thing wasn't handled properly and/or
with the required attention.
It's nobody's fault, but it ended up broken and I want to fix it
(possibly with the help of somebody to write the patch, because I'm
not a C programmer).

I'll write an RFC on it shortly and I hope it can get through before
the release.

Cheers,
Andrey.

Hi Andrey,
read_only mode is "read and finish session".
Shared lock is "can read session, but not write".

Semantics differs.

I like INIs. Therefore, it was INI initially.
It was made to start option, since others do not like INIs.
My common sense tells that there should be session_gc(), there should be
session_serializer_name() if there is session_module_name ;)

However, I like the idea of getting rid of INI modifying functions. I'm OK
with either
  - have both session_module_name() and session_serializer_name()
  - getting rid of them all

The RFC was made with long discussions. It involves other RFC, too. I may
miss to write some and it may be difficult to understand reason behind it.
I'll answer if there is questions. Please don't forget to write your reason
why feature should be changed.

Regards,

Hi Andrey,
I think shared lock is useful. It requires additional work to change save
handler interface and save handler code. It's a distinct feature from
read_only. You can get status via session_status() if you need to know
during execution. i.e. If session is started with read_only, you'll
get PHP_SESSION_NONE status.

I suppose read_only is good enough name for the feature. I appreciate
better name for it, if there is.

Although I would not like to propose/implement the RFC now, but I may
implement it if RFC passes.

Regards,

Hi,
This is a bug.
A read-only session is not a non-existing session.

I'm thinking more of something like a SessionHandler::$isReadOnly property.
Great, I'll count on that. :)

Hi Andrey,
This is not a bug.

session_start(['read_only'=>true])

is the same as

session_start();
session_commit();

It's much faster because no additional API call nor write to session
storage.


I have to think about the use case if it's really useful, but I may
implement shared lock if RFC passes.

Regards,

Hi Yasuo,
This is broken, consider the following (multiple tab/ajax/whatever
concurrency) scenario:

Request1: session_start(['read_only' => TRUE]);
Request2: session_start(); unset($_SESSION['logged_in']); session_commit();
Request1: still logged in

^ This screams "danger".

It's also, redundant ... do you really believe you'd get +1 votes for
that feature if the voters were *really* aware that this would be just
an alias for (sesson_start() && session_commit()) ?
I don't think so. This is simply not what "read only" means which
makes this whole thing very misleading and if people who were supposed
to review the feature got confused by it, imagine what happens in
userland.

Cheers,
Andrey.

That is _not_ broken WRT sessions themselves, because the readonly session
of request 1 will not be permitted to change session data (it is readonly)
and as such the situation is the same as if request 1 completed before
request 2 started.

It _is_ broken when the session lock is "misused" as a lock wrt. other,
nonsession data / database modifications / whatever else. In that case,
using the readonly session feature or any other kind of different-lifetime
session lock scheme is bad for the application - don't do that then.

best regards
   Patrick

Hi,
It is not broken functionally, indeed.
It's broken by design - if I write session_start(), options or not, I
would never expect it to immediately close the session. It's highly
misleading and this will lead to a lot of abuse.

Cheers,
Andrey.

Hi all,
I don't want to confuse users.
Better name would be appreciated.

Perhaps, "close" may be better option name.

session_start(['read_only'=>true]);
  ↓
session_start(['close']=>true);

Any comments/better names?

Regards,

Hi,
I'd rather suggest this to be a separate function and not an option
for session_start(). I've got this coverered in a draft RFC that I
will announce for discussion later today.

Now back to the main topic:

Please exclude session_serializer_name(), session_gc(),
session_reset(), session_abort() and the "session write short-circuit"
from the 5.6 branch.

Cheers,
Andrey.

Hi Andrey,
It could have dedicated function, but it is not needed.

I removed session_serializer_name() and session_gc()
(Although session_gc() is mandatory API, IMO)
I like the idea removing INI modifying function in the future release.

I don't understand reason why you insist removal of session_reset()
and session_abort(). They are just missing API for session module,
like session_gc().

There should be API (i.e. function/method or parameters) for distinct
operations that user may use.

I may agree if you could provide the reason why there should not be
these APIs.

Regards,

Hi all,
I thought it might be better to explain what new functions do.

session_gc() executes GC without tweaking INIs.
session_abort() aborts session without writing $_SESSION. There is no way
achieve w/o it.
session_reset() re-reads session and re-initializes $_SESSION. There is no
way achieve w/o it. (It could be done with session_abort(), though)
"write short circuit" omits "write" when $_SESSION hasn't change. There is
no point calling write API and writing to storage for the same data.

Regards,

Hi,
session_abort(), session_reset() are just two functions that somebody
asked about in 2002 via bugs.php.net and nobody responded (11 years
without a single comment) right up until you just assumed it's a good
idea and commited it. If it was "missing API", surely at least 1 user
per year would've requested it. ;)

I understand that you see them as small, trivial additions, but being
a part of sessions automatically makes them very important and as
such, they should be evaluated collectively, not by a single person.
"write short circuit" as I understand it, is an exact copy of the
'lazy_write' option. This will be addressed in the previously
mentioned RFC that I'll post later today.

Cheers,
Andrey.

It does not explain why it is not needed.

For example, session_abort() can be used to with error/exception during
execution.
You may not use, but I would use it to make sure $_SESSION would not
contains
any unneeded changes when error/exception occurred.

No. It's not.

"lazy_write" does not lock session data while "write short circuit" does.
In other words, "lazy_write" changes session behavior, but "write short
circuit" does not.
i.e. With locked session data, "write short circuit" would not change how
session manager works.

Regards,

Hi,
I'll just repeat what I've said previously: acceptance of a new
feature can't be justified by a lack of "why not" reasons, it should
work the other way around.
I also want to use a lot of features that don't currently exist in
PHP, but only few people like yourself have the privilege of being
able to commit them. However, everything should go through peer review
_before_ it gets in, not after.
I can't stress enough how confusing and potentially unsafe that is.

Cheers,
Andrey.

Hi Andrey,

You need to argue why missing APIs that are added aren't needed.
Just because they were missing years will not make them unneeded.

For example, there are few new function requests for mbstring. Just adding
missing APIs to _NEW_ release does not require RFC, simply making thing
more efficient and faster does not require RFC, making small internal BC
does
not require RFC, does it?

You are asking to make RFC always. (I didn't say discussion isn't needed)

Anyone can make pull requests now. I even send PR if there needs to have
some agreement. Anyone can comment committed patch.

It does not make difference since many users use session_commit()
to get more concurrency already. Simple documentation would be enough to
understand what it does.

There are freedom to shoot their own foot by themselves in any languages.
We have better performance/more complex operations/convenience in return.

Rather than discussing missing APIs are needed or not, I would like to
discuss
session_regenerate_id() issue. This is the design issue of session manager.
IMO.

Regards,

Hi Andrey,
One more comment for this.

"write short cut" is designed to work with old(I mean existing) save
handlers.
Behavior does _not_ change at all. New save handlers that support new save
handler API work much faster. That's it.

Save handlers like memcahe/memcached updates last access time stamp
by reading data. These save handler may omit writes at all when session data
hasn't changed. I don't think explanation is needed why this gives us better
performance.

Regards,

Hi Andrey,
I think all of us know what it does.

It's not alias. It's much more efficient.

I might not explain well enough in the RFC, but most of use understood it's
a faster version of session_start();session_commit().

Calling session_commit() right after session_start() is common optimization
technique. Making it more efficient worth it.

I think there is no point to write back to the session data while
programmer only needs to read the data. Since
session_start();session_commit(); is optimization technique, making it more
efficient makes sense.

Regards,

Pressed send button before re-reading.
I think it could be understood what I mean.

Hi Andrey,
Time stamp exists. It's correct partially.

The time stamp is not usable for HTTP session manager to manage session
properly. We may have new API that gets time stamp of session data, but it
would require large overhead. i.e. Need additional API call to get time
stamp = additional query to session storage. The overhead cannot be ignored.

The proper way to get and set the time stamp is "get/set time stamp" when
"store/retrieve session data". This way, there would not be overhead. User
may implement in their script, but it's a session manager task to begin
with.

Managing time stamp is the main task of session GC, isn't it?


You are the one who said there should be RFCs ;)
There are RFCs because I'm trying to change existing behaviors rather than
simple bug fixes.

Anyway, this was discussion of committed patch. It has much higher priority
than not implemented RFC, I suppose. I still think there should be GC
function. Programmers should control GC and there should be session
function for it rather than INI tweak.

https://wiki.php.net/rfc/secure-session-options-by-default

It's better to write something so that it would be forgotten.
I forgot to update session_regenerate_id RFC since it became part of other
RFC. I'll update it soon.

Regards,

Yasuo, you took 2 paragraphs that belong together and put them out of
context ...
Exactly, focus on current tasks is what I'm saying.
In the timespan of 5 hours you asked me twice if I will write the RFC
_and_ wrote it without even waiting for the reply.

Let's stop this nonsense now and discuss more important stuff. :)

Cheers,
Andrey.

I've not had much interest in this discussion as I am more than happy with how
things work now. It is not unusual for my 'client' sessions to last for an hour
or so without activity and their 'automatically' logging out was the problem
originally. So now I'm set never to clear session data and run a clear up on the
session directory before the site starts work in the morning. There are reasons
a system wide approach may be more practical than changing how a single session
handles things?

Hi Andrey,

I think it good to document them now.
I don't mind at all adding E_DEPRECATED for session_module_name(),
mb_regex_encoding(), etc for PHP 5.7+.

Could you create the RFC?

Regards,

Hi Andrey,
I've created RFC

https://wiki.php.net/rfc/deprecate-ini-functions

Please feel free to update it.

Regards,

Hello guys,

When we branched 5.6, we branched it from master, with these commits into
it.
Ferenc did a hard job to spot what was in master at this time, and it looks
like he didn't notice those addings.

We should revert them.

We *cannot* accept new functions in 5.6 , if the community in its whole,
does not agree with them, by voting an RFC.
I'm sorry, but that is our policy, and we are not talking about "little"
functions we could blindly accept (because we are not bad dragons neither
:-p, some things are tiny and can be merged without heavy RFC process).
Those changes impact the session module, which is an important one.

Yasuo : I know you are actually working on RFCs for sessions for 5.6. I
would love to see them accepted and merged to 5.6. From what I know, one
has already been accepted partially (
https://wiki.php.net/rfc/session-lock-ini ). It seems like it's not been
merged yet ?
Is it because it cant, like you seem to say in
http://markmail.org/message/dcykcmq6exvkflf7 ?
Please, also remember that we are going to tag beta1 on 18th , your RFCs
need to be accepted and merged for this date.


Having said that, I agree we should clean the session API.
I agree session_module_name() is very uncommon and likely not very used,
and it should dissapear.
I agree as well that having to touch INI settings with ini_set() to change
the session behavior is not really nice, and that those should be feasable
using functions, or arguments to session_start();
However, all those points are BC breaking points, and can not be addressed
for 5.6 ; but I encourage all of you to start discussions about the session
module and those points, for PHP6.

Thank you all for your understanding, we all want to make PHP better, but
we all are humans :-p

Julien.Pauli