[RFC] Revert/extend/postpone original RFC about read_only, lazy_write sessions

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

"lazy_write" is not accepted. So you don't need to mention it in the RFC.

"read_only" option is better/much faster version of "session_start();
session_commit()".
If the name "read_only" is confusing, better name would be appreciated.

BTW, I have to commit accepted RFC change now. Option name may be changed
before release. "write short cut" is there, since it does not affect
existing behavior
nor save handlers at all.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

"lazy_write" is not accepted. So you don't need to mention it in the RFC.

Yes, it is.
People (including yourself) have voted +1 for "Read only, lazy write option".

"read_only" option is better/much faster version of "session_start();
session_commit()".
If the name "read_only" is confusing, better name would be appreciated.

I know what it is, Yasuo ... half of the RFC is about it, please read
it carefully.

"write short cut" is there, since it does not affect
existing behavior
nor save handlers at all.

The public probably doesn't know what "write short cut" is, because
it's only mentioned in a separate discussion.
So, for the public, it is this: https://bugs.php.net/bug.php?id=17860

... or in other words, a non-optional version of 'lazy_write'.

It does affect current functionality ... it's described in the RFC as
'lazy_write'.

Regards,
Andrey.

10 years ago by Stas Malyshev — view source

unread

Hi!

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

Frankly, I'm not sure what is the point of this - submitting RFC that
says "let's throw out RFC that we just accepted". If somebody didn't
want that RFC to happen, he'd vote against it, no?

I also don't understand what's the problem with read only option. Read
only option means only one thing - you're going to read the session, but
not write into it. Hence read only. Your scenario with login check is
not a problem because all session data are always read at the beginning
of the session. If another session changes the state later, it in any
case can not influence the session that have already read the data. The
only change is that now the session can say "I am not going to change
the state, nothing I am doing should change the state, so I'm not going
to waste my time on writing anything back".
Not allowing $_SESSION to be written sounds pretty useless to me, since
it is not going to be written anywhere anyhow, but this option does not
prevent implementing it in any way.

As for lazy_write, incomplete is not the reason to revert accepted
feature. Anything can be improved and amended, if we rejected features
because they could be improved, we'd never add anything. There is a use
case for this feature to exist, as an extension of the read only option

it is a case where the session might change the state, but not
always does. So you have three options now:

My request is never going to change the state => use read_only
My request might change the state and thus I need exclusive lock =>
use lazy_write
My request always changes the state => use defaults

The difference between 1 and 2 is shorter period under lock, between 2
and 3 - saving time on transmitting data over the wire that are already
in session storage anyway. As for UPDATE_FUNC, I understand this is only
for updating "last used" if it exists and may be defaulted to nothing.

Again, nothing prevents us in PHP 6 from changing the defaults and
making this option part of the default option set.

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

"lazy_write" is not accepted. So you don't need to mention it in the RFC.

The vote result says "yes" on "Read only, lazy write option".

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

10 years ago by Yasuo Ohgaki — view source

unread

On Sun, Mar 16, 2014 at 10:28 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

"lazy_write" is not accepted. So you don't need to mention it in the RFC.

The vote result says "yes" on "Read only, lazy write option".

Thank you Stas.
There are long discussions and I mixed with "unsafe_lock" myself...

I'll prepare the patch now and put it in github today.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

Hi!

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

Frankly, I'm not sure what is the point of this - submitting RFC that
says "let's throw out RFC that we just accepted". If somebody didn't
want that RFC to happen, he'd vote against it, no?

Well, as I said: I don't think the previous RFC was handled properly.
This is probably because it isn't detailed enough for people to
understand it (I for example wouldn't vote for, or against something
that I don't understand). The discussion itself happened during the
holiday season when most people wouldn't care much about work, if at
all - this is also another factor that could've contributed.
Either way, I feel that this is important, so I wrote an RFC.

I also don't understand what's the problem with read only option. Read
only option means only one thing - you're going to read the session, but
not write into it. Hence read only. Your scenario with login check is
not a problem because all session data are always read at the beginning
of the session. If another session changes the state later, it in any
case can not influence the session that have already read the data. The
only change is that now the session can say "I am not going to change
the state, nothing I am doing should change the state, so I'm not going
to waste my time on writing anything back".

Yes, on a basic level the currently accepted 'read_only' matches your
explanation. But there's more to it than that ... options should be
viewed as modes of operation (IMO anyway), and currently this is an
"option" that actually performs an action, and that's not even
obvious. 'read_only' just doesn't have the same meaning as
'read_and_close', which because is an action, should be a separate
function.

As for the login check example, maybe it isn't clear enough. What it
demonstrates is a decision making process, you don't know what happens
afterwards. What if, for example, Request2 deletes a row from a
database at the time of logout and Request1 tries to read the same row
(knowing that it is there because the user is logged in)? The
application breaks.

Not allowing $_SESSION to be written sounds pretty useless to me, since
it is not going to be written anywhere anyhow, but this option does not
prevent implementing it in any way.

How useful that would be is not the point, currently. The problem is
that this is exactly what 'read_only' should mean as an option/mode.
If that name is taken, how would you call it in the future?

As for lazy_write, incomplete is not the reason to revert accepted
feature. Anything can be improved and amended, if we rejected features
because they could be improved, we'd never add anything. There is a use
case for this feature to exist, as an extension of the read only option

it is a case where the session might change the state, but not
always does. So you have three options now:

I didn't mean incomplete in the sense of "it works, but can be
better", but rather as "it only works in half" or "we don't know how
it works in some cases". The RFC itself is incomplete, it doesn't
address a lot of potential issues. It's like knowing how to lock a
door, but not how to unlock it.
I'm not questioning the use cases, don't get me wrong - I want this
feature. It just looks like a prototype and not a real solution.

My request is never going to change the state => use read_only

My request might change the state and thus I need exclusive lock =>
use lazy_write

My request always changes the state => use defaults

The difference between 1 and 2 is shorter period under lock, between 2
and 3 - saving time on transmitting data over the wire that are already
in session storage anyway. As for UPDATE_FUNC, I understand this is only
for updating "last used" if it exists and may be defaulted to nothing.

"last used", "last updated", "last modified", "last accessed" ...
however you call it, it always exists in some form. Without such a
timestamp, there's no way to decide if a session has expired or not,
so you can't default to nothing ... Something has to update it and
that something may be added to "stock" session handlers, but doesn't
exist in third-party or userland ones - this is where it breaks.

Again, nothing prevents us in PHP 6 from changing the defaults and
making this option part of the default option set.

I didn't say there is, just that it's more appropriate to leave it for then.

Cheers,
Andrey.

10 years ago by Stas Malyshev — view source

unread

Hi!

Well, as I said: I don't think the previous RFC was handled properly.
This is probably because it isn't detailed enough for people to
understand it (I for example wouldn't vote for, or against something

Are there any other people that you know that think so? Because it makes
a precedent for never ending RFCing - somebody creates RFC and holds a
vote, you don't like the result so you create counter-RFC, then they
create counter-counter-RFC and in the meantime nothing gets done. There
should be a point where the decision is taken. That doesn't mean we
can't reconsider, but not a minute after the vote ends.

Yes, on a basic level the currently accepted 'read_only' matches your
explanation. But there's more to it than that ... options should be
viewed as modes of operation (IMO anyway), and currently this is an
"option" that actually performs an action, and that's not even
obvious. 'read_only' just doesn't have the same meaning as
'read_and_close', which because is an action, should be a separate
function.

I'm not sure I understand your complaint. We already had the
session_abort function, and nothing prevents you from using it if that's
what you want. What you couldn't previously do is to quickly say "I'm
not going to change state" and be done with it. It is a real use case
and the RFC enables it. We already have all the components
(session_abort), we just enable doing it in a clear and explicit manner
that allows people reading the code know what's going on.

As for the login check example, maybe it isn't clear enough. What it
demonstrates is a decision making process, you don't know what happens
afterwards. What if, for example, Request2 deletes a row from a
database at the time of logout and Request1 tries to read the same row
(knowing that it is there because the user is logged in)? The
application breaks.

Right, you can write app with broken logic (like assuming the DB row is
there because of something that is not related to the DB row and can be
changed independently). I don't see how this relates to read_only - you
always could and always will be able to write apps with broken logic.

How useful that would be is not the point, currently. The problem is
that this is exactly what 'read_only' should mean as an option/mode.
If that name is taken, how would you call it in the future?

I don't see any reason why this option "should mean" what you claim it
should mean. That's just your misunderstanding of what it does, easily
rectified by a quick look into the docs.

I didn't mean incomplete in the sense of "it works, but can be
better", but rather as "it only works in half" or "we don't know how
it works in some cases". The RFC itself is incomplete, it doesn't

Where we do not know how it works? The feature is very simple - when
session data is about to be written, we compare it to the session data
that has been read, and if they are identical, we do not issue write.
Not many cases here as far as I can see.

"last used", "last updated", "last modified", "last accessed" ...
however you call it, it always exists in some form. Without such a

Last used and last modified are very different things. Nothing prevents
one from tracking either. We need to check it is done properly and is
compatible with old handlers, but it should be entirely possible to do
it in a BC way.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

10 years ago by Andrey Andreev — view source

unread

Hi,

Hi!

Well, as I said: I don't think the previous RFC was handled properly.
This is probably because it isn't detailed enough for people to
understand it (I for example wouldn't vote for, or against something

Are there any other people that you know that think so? Because it makes
a precedent for never ending RFCing - somebody creates RFC and holds a
vote, you don't like the result so you create counter-RFC, then they
create counter-counter-RFC and in the meantime nothing gets done. There
should be a point where the decision is taken. That doesn't mean we
can't reconsider, but not a minute after the vote ends.

I understand your concern, but making a precedent is not the goal. If
this can be handled without an RFC, I'll withdraw it. However, it's
also possible that somebody says "this has gone through the RFC
process and was voted, so only another vote can override it". It's
also an RFC because it's easier to read from a single place and not
get lost in discussions.

Yes, on a basic level the currently accepted 'read_only' matches your
explanation. But there's more to it than that ... options should be
viewed as modes of operation (IMO anyway), and currently this is an
"option" that actually performs an action, and that's not even
obvious. 'read_only' just doesn't have the same meaning as
'read_and_close', which because is an action, should be a separate
function.

I'm not sure I understand your complaint. We already had the
session_abort function, and nothing prevents you from using it if that's
what you want. What you couldn't previously do is to quickly say "I'm
not going to change state" and be done with it. It is a real use case
and the RFC enables it. We already have all the components
(session_abort), we just enable doing it in a clear and explicit manner
that allows people reading the code know what's going on.

What's there to understand? Just look at it:

session_start(['read_only' => TRUE])

It's read as "start for reading only", and not "start read an close",
which what it actually does. I strongly disagree that this is "clear
and explicit".

Nowhere have I said that there's no use case. I'm questioning the API
design, not the feature itself, the proposal is not necessarily to
drop a feature. Yasuo (as an author of the whole thing) already
offered to change the name, so why are we even arguing over this?

As for the login check example, maybe it isn't clear enough. What it
demonstrates is a decision making process, you don't know what happens
afterwards. What if, for example, Request2 deletes a row from a
database at the time of logout and Request1 tries to read the same row
(knowing that it is there because the user is logged in)? The
application breaks.

Right, you can write app with broken logic (like assuming the DB row is
there because of something that is not related to the DB row and can be
changed independently). I don't see how this relates to read_only - you
always could and always will be able to write apps with broken logic.

Sure you can, but it's harder to make that mistake if you have to call
session_start_close() instead.

How useful that would be is not the point, currently. The problem is
that this is exactly what 'read_only' should mean as an option/mode.
If that name is taken, how would you call it in the future?

I don't see any reason why this option "should mean" what you claim it
should mean. That's just your misunderstanding of what it does, easily
rectified by a quick look into the docs.

Because, it has the same meaning everywhere. Type 'define: read-only'
in google and it would show the same meaning that I'm saying it
should.

I didn't mean incomplete in the sense of "it works, but can be
better", but rather as "it only works in half" or "we don't know how
it works in some cases". The RFC itself is incomplete, it doesn't

Where we do not know how it works? The feature is very simple - when
session data is about to be written, we compare it to the session data
that has been read, and if they are identical, we do not issue write.
Not many cases here as far as I can see.

This is all explained in the RFC.

If there's no write, how do you know the session wouldn't expire
(since it's last_whatever timestamp isn't changed) because of omitting
the write call? OK, call PS_UPDATE_FUNC() instead ... but there's no
SessionHandler()::update(), SessionHandlerInterface::update(), so what
do we do then? Nobody knows that.

In fact, in the very next quote, you're saying that it needs to be
checked, which means that you don't currently know that:

"last used", "last updated", "last modified", "last accessed" ...
however you call it, it always exists in some form. Without such a

Last used and last modified are very different things. Nothing prevents
one from tracking either. We need to check it is done properly and is
compatible with old handlers, but it should be entirely possible to do
it in a BC way.

See, this is what I meant with 'read_only' it may be a name closely
explaining what it does, but in fact it has an essentially different
meaning. :) The session module only uses one of those timestamps is
what I meant here. And it may be possible to handle that in a BC way,
but the RFC doesn't mention if that's really the case and/or how it
could be done - this is stuff that must be addressed.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

"last used", "last updated", "last modified", "last accessed" ...
however you call it, it always exists in some form. Without such a

Last used and last modified are very different things. Nothing prevents
one from tracking either. We need to check it is done properly and is
compatible with old handlers, but it should be entirely possible to do
it in a BC way.

See, this is what I meant with 'read_only' it may be a name closely
explaining what it does, but in fact it has an essentially different
meaning. :) The session module only uses one of those timestamps is
what I meant here. And it may be possible to handle that in a BC way,
but the RFC doesn't mention if that's really the case and/or how it
could be done - this is stuff that must be addressed.

It updates time stamp always.

It does

"write data if data is updated"
or
"write data if save handler does not have update API"
or
"update time stamp if it is needed" (e.g. memcache does not need to update
time stamp since it updates time stamp by read. It's save handler
implementation choice what to do with update API)

For save handlers like memcahe/memcached, session module is writing back
the same data just to waste resources when session data hasn't changed.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

Hi Andrey,

"last used", "last updated", "last modified", "last accessed" ...
however you call it, it always exists in some form. Without such a

Last used and last modified are very different things. Nothing prevents
one from tracking either. We need to check it is done properly and is
compatible with old handlers, but it should be entirely possible to do
it in a BC way.

See, this is what I meant with 'read_only' it may be a name closely
explaining what it does, but in fact it has an essentially different
meaning. :) The session module only uses one of those timestamps is
what I meant here. And it may be possible to handle that in a BC way,
but the RFC doesn't mention if that's really the case and/or how it
could be done - this is stuff that must be addressed.

It updates time stamp always.

It does

"write data if data is updated"
or
"write data if save handler does not have update API"
or
"update time stamp if it is needed" (e.g. memcache does not need to update
time stamp since it updates time stamp by read. It's save handler
implementation choice what to do with update API)

For save handlers like memcahe/memcached, session module is writing back the
same data just to waste resources when session data hasn't changed.

And what about userland session handlers?

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

It updates time stamp always.

It does

"write data if data is updated"
or
"write data if save handler does not have update API"
or
"update time stamp if it is needed" (e.g. memcache does not need to
update
time stamp since it updates time stamp by read. It's save handler
implementation choice what to do with update API)

For save handlers like memcahe/memcached, session module is writing back
the
same data just to waste resources when session data hasn't changed.

And what about userland session handlers?

It's the same as C written module. Unless there is update() method, session
module
writes session data always.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

Hi Andrey,

It updates time stamp always.

It does

"write data if data is updated"
or
"write data if save handler does not have update API"
or
"update time stamp if it is needed" (e.g. memcache does not need to
update
time stamp since it updates time stamp by read. It's save handler
implementation choice what to do with update API)

For save handlers like memcahe/memcached, session module is writing back
the
same data just to waste resources when session data hasn't changed.

And what about userland session handlers?

It's the same as C written module. Unless there is update() method, session
module
writes session data always.

See, that's where it gets tricky ...

Is there a SessionHandlerInterface::update() method? (I assume not,
otherwise it would be enforced)

Is there a default SessionHandler::update() method? If yes, how is
that handled? That's supposedly just exposing the internal API and a
user can choose whether to override one or many methods. But if all of
them are overriden, how do you know it uses the same storage at all?
And what if an existing userland handler already has an update()
method that does something completely different?

The devil is in the details and these details aren't explained
anywhere, that's why I'm concerned.

Btw, this should probably be called updateTimestamp() instead of
update(), for userland at least. write() vs. update() can be somewhat
confusing.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

See, that's where it gets tricky ...

Is there a SessionHandlerInterface::update() method? (I assume not,
otherwise it would be enforced)

Session module handles interface a little different way.
There is create_sid() method for a long time, but it's not forced to be
implemented. validateSid() and update() method is the same as create_sid().

Is there a default SessionHandler::update() method? If yes, how is

that handled? That's supposedly just exposing the internal API and a
user can choose whether to override one or many methods. But if all of
them are overriden, how do you know it uses the same storage at all?
And what if an existing userland handler already has an update()
method that does something completely different?

There is dummy function for validateSid() and update().
Both returns true always.

If save handler (regardless of user or C) does not implement its own
update(),
then update() will not be called, instead write() is called.
To do that, address of the dummy function is compared.

The devil is in the details and these details aren't explained

anywhere, that's why I'm concerned.

Currently implemented save handlers work as it is now both user and C.

Btw, this should probably be called updateTimestamp() instead of

update(), for userland at least. write() vs. update() can be somewhat
confusing.

This could be changed. Better name is always good.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

Hi Andrey,

See, that's where it gets tricky ...

Is there a SessionHandlerInterface::update() method? (I assume not,
otherwise it would be enforced)

Session module handles interface a little different way.
There is create_sid() method for a long time, but it's not forced to be
implemented. validateSid() and update() method is the same as create_sid().

I believe I already replied to this previously ... *sid() is passed to
the other functions, so that's not really relevant. But ok, I get the
idea.

Is there a default SessionHandler::update() method? If yes, how is
that handled? That's supposedly just exposing the internal API and a
user can choose whether to override one or many methods. But if all of
them are overriden, how do you know it uses the same storage at all?
And what if an existing userland handler already has an update()
method that does something completely different?

There is dummy function for validateSid() and update().
Both returns true always.

If save handler (regardless of user or C) does not implement its own
update(),
then update() will not be called, instead write() is called.
To do that, address of the dummy function is compared.

Ok, so why is 'lazy_write' an option at all then? I don't see a reason
why it shouldn't be the default and even mandatory behavior.

The devil is in the details and these details aren't explained
anywhere, that's why I'm concerned.

Currently implemented save handlers work as it is now both user and C.

Btw, this should probably be called updateTimestamp() instead of
update(), for userland at least. write() vs. update() can be somewhat
confusing.

This could be changed. Better name is always good.

Well, there you go - updateTimestamp() is a better name. ;)

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi all,

On Mon, Mar 17, 2014 at 6:14 PM, Andrey Andreev narf@devilix.net
wrote:

Is there a default SessionHandler::update() method? If yes, how is
that handled? That's supposedly just exposing the internal API and a
user can choose whether to override one or many methods. But if all of
them are overriden, how do you know it uses the same storage at all?
And what if an existing userland handler already has an update()
method that does something completely different?

There is dummy function for validateSid() and update().
Both returns true always.

If save handler (regardless of user or C) does not implement its own
update(),
then update() will not be called, instead write() is called.
To do that, address of the dummy function is compared.

Ok, so why is 'lazy_write' an option at all then? I don't see a reason
why it shouldn't be the default and even mandatory behavior.

I would like to make 'lazy_write' default to TRUE, since there would not be
'unsafe_lock'.
I wouldn't try to add 'unsafe_lock' anymore. It's safe to make 'lazy_write'
default to TRUE.

'lazy_write' needs a little memory, but it's session. Session data would be
small enough
for almost all apps and memory is cheap these days.
(Note: I've implemented 'lazy_write' by using MD5 hash at first, but
benchmark revealed
simple store and string compare is much faster. It's saving loaded session
data and
compare to check modification with new patch.)

Any objection/comment make this default to TRUE or automatically apply
'lazy_write'?
I think automatically applying 'lazy_write' is good choice.

The devil is in the details and these details aren't explained

anywhere, that's why I'm concerned.

Currently implemented save handlers work as it is now both user and C.

Btw, this should probably be called updateTimestamp() instead of
update(), for userland at least. write() vs. update() can be somewhat
confusing.

This could be changed. Better name is always good.

Well, there you go - updateTimestamp() is a better name. ;)

I thought of similar name. I choose update() since it seemed it is matching
name for read()/write(), but descriptive name may be better.

Any comments for renaming?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

If save handler (regardless of user or C) does not implement its own
update(),
then update() will not be called, instead write() is called.
To do that, address of the dummy function is compared.

Ok, so why is 'lazy_write' an option at all then? I don't see a reason
why it shouldn't be the default and even mandatory behavior.

I would like to make 'lazy_write' default to TRUE, since there would not be
'unsafe_lock'.
I wouldn't try to add 'unsafe_lock' anymore. It's safe to make 'lazy_write'
default to TRUE.

'lazy_write' needs a little memory, but it's session. Session data would be
small enough
for almost all apps and memory is cheap these days.
(Note: I've implemented 'lazy_write' by using MD5 hash at first, but
benchmark revealed
simple store and string compare is much faster. It's saving loaded session
data and
compare to check modification with new patch.)

Any objection/comment make this default to TRUE or automatically apply
'lazy_write'?
I think automatically applying 'lazy_write' is good choice.

My quesion was rather why does it need to be optional at all
(regardless of the default value)? If it is somehow handled at all
times, it becomes just a performance improvement. I don't see a reason
why performance improvements should be optional.

Btw, this should probably be called updateTimestamp() instead of
update(), for userland at least. write() vs. update() can be somewhat
confusing.

This could be changed. Better name is always good.

Well, there you go - updateTimestamp() is a better name. ;)

I thought of similar name. I choose update() since it seemed it is matching
name for read()/write(), but descriptive name may be better.

Any comments for renaming?

I'm all for it (naturally, since I suggested it), it could be
updateTimestamp(), updateTs(), updateTime(), etc. Anything in that
fashion would be more descriptive and less confusing.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi all,

If save handler (regardless of user or C) does not implement its own
update(),
then update() will not be called, instead write() is called.
To do that, address of the dummy function is compared.

Ok, so why is 'lazy_write' an option at all then? I don't see a reason
why it shouldn't be the default and even mandatory behavior.

I would like to make 'lazy_write' default to TRUE, since there would not
be
'unsafe_lock'.
I wouldn't try to add 'unsafe_lock' anymore. It's safe to make
'lazy_write'
default to TRUE.

'lazy_write' needs a little memory, but it's session. Session data would
be
small enough
for almost all apps and memory is cheap these days.
(Note: I've implemented 'lazy_write' by using MD5 hash at first, but
benchmark revealed
simple store and string compare is much faster. It's saving loaded
session
data and
compare to check modification with new patch.)

Any objection/comment make this default to TRUE or automatically apply
'lazy_write'?
I think automatically applying 'lazy_write' is good choice.

My quesion was rather why does it need to be optional at all
(regardless of the default value)? If it is somehow handled at all
times, it becomes just a performance improvement. I don't see a reason
why performance improvements should be optional.

I agree.
If nobody objects, I'll remove the option. Please send mail ASAP!

Btw, this should probably be called updateTimestamp() instead of
update(), for userland at least. write() vs. update() can be somewhat
confusing.

This could be changed. Better name is always good.

Well, there you go - updateTimestamp() is a better name. ;)

I thought of similar name. I choose update() since it seemed it is
matching
name for read()/write(), but descriptive name may be better.

Any comments for renaming?

I'm all for it (naturally, since I suggested it), it could be
updateTimestamp(), updateTs(), updateTime(), etc. Anything in that
fashion would be more descriptive and less confusing.

I agree this, too.
I would choose "updateTimestamp".
Please send mail ASAP, if anyone has comment.

When is the beta due? I made PR so that RMs can merge it.
I don't need git commit log. It's just a personal memo. Please feel free to
merge
with squash if RMs are would like to merge it now. It's good enough for
beta.

https://github.com/php/php-src/pull/628

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Stas Malyshev — view source

unread

Hi!

I agree.
If nobody objects, I'll remove the option. Please send mail ASAP!

I think defaults should not change in minor version, unless there's a
very good reason for it, and here it's not a very good reason. Session
module is very sensitive, and doing changes in it should be very
conservative to not break people's workflows. While new behavior serves
an important use case, it may not be everybody's use case, especially
with all handlers. Thus, I would rather have it an option - exactly as
voted. When we move into PHP 6, where certain major changes are
expected, we can change the defaults. But I think introducing such
change in a minor version is unnecessary risk.
And also it subverts RFC process as it's not what the vote was for, and
it's a substantial change - default behavior change (means, everybody
affected) instead of option (means, only people using the option affected).

When is the beta due? I made PR so that RMs can merge it.

I'd recommend waiting for it to be thoroughly reviewed before merging. I
myself will try to take a look in coming days, as my schedule allows,
but it may take time to polish some things. Sessions are very sensitive
area, so we need to be very careful.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

10 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

Thank you for the comment on github.

On Tue, Mar 18, 2014 at 2:45 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:

I agree.
If nobody objects, I'll remove the option. Please send mail ASAP!

I think defaults should not change in minor version, unless there's a
very good reason for it, and here it's not a very good reason. Session
module is very sensitive, and doing changes in it should be very
conservative to not break people's workflows. While new behavior serves
an important use case, it may not be everybody's use case, especially
with all handlers. Thus, I would rather have it an option - exactly as
voted. When we move into PHP 6, where certain major changes are
expected, we can change the defaults. But I think introducing such
change in a minor version is unnecessary risk.
And also it subverts RFC process as it's not what the vote was for, and
it's a substantial change - default behavior change (means, everybody
affected) instead of option (means, only people using the option affected

No problem.

How about making it a INI option? It makes session_start() option handling
code
a little simpler. It's not mandatory, though.

When is the beta due? I made PR so that RMs can merge it.

I'd recommend waiting for it to be thoroughly reviewed before merging. I
myself will try to take a look in coming days, as my schedule allows,
but it may take time to polish some things. Sessions are very sensitive
area, so we need to be very careful.

I agree. Even for beta, we should be careful.

I found issue with object based user save handler. Unlike procedural user
save handlers,
object based user save handler uses previously used save handler as it's
base class (some what)

I think we are better to have another SessionHandler object that support
new APIs.
We can handle create_sid() method rename with new object also. We may keep
current implementation undocumented and may document it new one
(createSid()) only.
I will name it "SessionUpdateTimestampHandler". If anyone has suggestions,
I would appreciate it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

I think defaults should not change in minor version, unless there's a
very good reason for it, and here it's not a very good reason. Session

I don't think 5.6 is looked at as a minor version. Features have been
added, changed, deprecated and removed in previous 5.x releases.

module is very sensitive, and doing changes in it should be very
conservative to not break people's workflows. While new behavior serves
an important use case, it may not be everybody's use case, especially
with all handlers. Thus, I would rather have it an option - exactly as
voted. When we move into PHP 6, where certain major changes are
expected, we can change the defaults. But I think introducing such
change in a minor version is unnecessary risk.
And also it subverts RFC process as it's not what the vote was for, and
it's a substantial change - default behavior change (means, everybody
affected) instead of option (means, only people using the option affected

Exacly why I wrote an RFC, except Yasuo already explained that current
workflows (speaking of userland handlers) are not affected and BC is
maintaned.

No problem.

How about making it a INI option? It makes session_start() option handling
code
a little simpler. It's not mandatory, though.

+1 from me, if it is to be kept as an option.

I found issue with object based user save handler. Unlike procedural user
save handlers,
object based user save handler uses previously used save handler as it's
base class (some what)

Isn't that the whole point of it, that you can overload only specific
parts of the default one?

I think we are better to have another SessionHandler object that support new
APIs.
We can handle create_sid() method rename with new object also. We may keep
current implementation undocumented and may document it new one
(createSid()) only.
I will name it "SessionUpdateTimestampHandler". If anyone has suggestions,
I would appreciate it.

I object, although it should've been called SessionFilesHandler in the
first place, that way we could also have SessionMemcacheHandler,
SessionWhateverHandler - way nicer than it currently is, but again -
not the point of this discussion.

Cheers,
Andrey.

10 years ago by Ferenc Kovacs — view source

unread

2014.03.18. 11:59, "Andrey Andreev" narf@devilix.net ezt írta:

Hi,

I think defaults should not change in minor version, unless there's a
very good reason for it, and here it's not a very good reason. Session

I don't think 5.6 is looked at as a minor version.

It should be. That was the whole concept of the "new" release process rfc;
more frequent minors which complies with the semantic versioning.

Features have been
added, changed, deprecated and removed in previous 5.x releases.

Yeah, we were in the transition to the new way of doing things.
5.4 broke less bc than 5.3, 5.5 less than 5.4 and 5.6 should be close to
zero except warnings, bugfixes and changes related to security.

10 years ago by Yasuo Ohgaki — view source

unread

Hi all,

Exacly why I wrote an RFC, except Yasuo already explained that current
workflows (speaking of userland handlers) are not affected and BC is
maintaned.

I found issue with object based save handlers, but it was solved without
any BC.

I also added createSid() method support. If both create_sid() and
createSid() exists,
createSid() is used.

Since user does not have to user Session module defined interface, user may
omit create_sid() definition.

Even without createSid() or my patch, current behavior is some what
confusing.
I'll document them all later.

No problem.

How about making it a INI option? It makes session_start() option
handling
code
a little simpler. It's not mandatory, though.

+1 from me, if it is to be kept as an option.

I'm about to modify patch to make it INI. No objections for this?

I found issue with object based user save handler. Unlike procedural user
save handlers,
object based user save handler uses previously used save handler as it's
base class (some what)

Isn't that the whole point of it, that you can overload only specific
parts of the default one?

It's solved. I should have try to prevent defining bogus handlers.
i.e. Existing C written save handlers do not support new API and object
based
save handler try to call it if user hasn't define method for these.

I think we are better to have another SessionHandler object that support
new
APIs.
We can handle create_sid() method rename with new object also. We may
keep
current implementation undocumented and may document it new one
(createSid()) only.
I will name it "SessionUpdateTimestampHandler". If anyone has
suggestions,
I would appreciate it.

I object, although it should've been called SessionFilesHandler in the
first place, that way we could also have SessionMemcacheHandler,
SessionWhateverHandler - way nicer than it currently is, but again -
not the point of this discussion.

I removed new object. It's not needed anymore, since I prevented bogus
methods
registration.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi all,

Exacly why I wrote an RFC, except Yasuo already explained that current
workflows (speaking of userland handlers) are not affected and BC is
maintaned.

I found issue with object based save handlers, but it was solved without any
BC.

I also added createSid() method support. If both create_sid() and
createSid() exists,
createSid() is used.

I thought this was currently in discussion, and in a separate thread?

How about making it a INI option? It makes session_start() option
handling
code
a little simpler. It's not mandatory, though.

+1 from me, if it is to be kept as an option.

I'm about to modify patch to make it INI. No objections for this?

Well, I said 'if' we keep it as an option at all. I'd rather update
the RFC to propose it as non-optional behavior since there are no
downsides from it, as I understand.

I think we are better to have another SessionHandler object that support
new
APIs.
We can handle create_sid() method rename with new object also. We may
keep
current implementation undocumented and may document it new one
(createSid()) only.
I will name it "SessionUpdateTimestampHandler". If anyone has
suggestions,
I would appreciate it.

I object, although it should've been called SessionFilesHandler in the
first place, that way we could also have SessionMemcacheHandler,
SessionWhateverHandler - way nicer than it currently is, but again -
not the point of this discussion.

I removed new object. It's not needed anymore, since I prevented bogus
methods
registration.

Bogus methods? I wrote this RFC because it was completely unclear how
certain situations would be handled ... now this is also unclear. :)
I sent you a private e-mail earlier today, perhaps you could explain
me that in a reply to it so that we don't spam the list.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

Hi all,

On Tue, Mar 18, 2014 at 7:58 PM, Andrey Andreev narf@devilix.net
wrote:

Exacly why I wrote an RFC, except Yasuo already explained that current
workflows (speaking of userland handlers) are not affected and BC is
maintaned.

I found issue with object based save handlers, but it was solved without
any
BC.

I also added createSid() method support. If both create_sid() and
createSid() exists,
createSid() is used.

I thought this was currently in discussion, and in a separate thread?

How about making it a INI option? It makes session_start() option
handling
code
a little simpler. It's not mandatory, though.

+1 from me, if it is to be kept as an option.

I'm about to modify patch to make it INI. No objections for this?

Well, I said 'if' we keep it as an option at all. I'd rather update
the RFC to propose it as non-optional behavior since there are no
downsides from it, as I understand.

How should we handle this?
It was session_start() option e.g. session_start(['lazy_write'=>true]);
We are proposing make it INI option.

Since session_start() accepts all INI options, so it can work as
session_start(['lazy_write'=>true]);
with INI.

Anyone?

I think we are better to have another SessionHandler object that
support
new
APIs.
We can handle create_sid() method rename with new object also. We may
keep
current implementation undocumented and may document it new one
(createSid()) only.
I will name it "SessionUpdateTimestampHandler". If anyone has
suggestions,
I would appreciate it.

I object, although it should've been called SessionFilesHandler in the
first place, that way we could also have SessionMemcacheHandler,
SessionWhateverHandler - way nicer than it currently is, but again -
not the point of this discussion.

I removed new object. It's not needed anymore, since I prevented bogus
methods
registration.

Bogus methods? I wrote this RFC because it was completely unclear how
certain situations would be handled ... now this is also unclear. :)
I sent you a private e-mail earlier today, perhaps you could explain
me that in a reply to it so that we don't spam the list.

It works as it is now.
Please refer to code for details.

https://github.com/php/php-src/pull/628/

It's mostly done.

Anyway, object based save handler registers "previous save handler"
function as
base methods. It may call bogus method for user handler.

Previous save handler may not have new API
Calling base class method is invalid. i.e. Overriding open() etc.

I made new API optional completely. Therefore, current save handler
works as it is now and users may define new API if they want.
Existing code (5.5 and less) has many protections against bogus method
calls, too.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

How about making it a INI option? It makes session_start() option
handling
code
a little simpler. It's not mandatory, though.

+1 from me, if it is to be kept as an option.

I'm about to modify patch to make it INI. No objections for this?

Well, I said 'if' we keep it as an option at all. I'd rather update
the RFC to propose it as non-optional behavior since there are no
downsides from it, as I understand.

How should we handle this?
It was session_start() option e.g. session_start(['lazy_write'=>true]);
We are proposing make it INI option.

Since session_start() accepts all INI options, so it can work as
session_start(['lazy_write'=>true]);
with INI.

Anyone?

I'll rephrase ... I'm proposing lazy_write at all times, no option.
Optional performance improvements don't make sense to me.

Please refer to code for details.

https://github.com/php/php-src/pull/628/

Wooow, that includes way more additions than those related to the
session-lock-ini RFC. Does it have to be all in one patch?

I'll comment further on github to avoid more spam here.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

Please refer to code for details.

https://github.com/php/php-src/pull/628/

Wooow, that includes way more additions than those related to the
session-lock-ini RFC. Does it have to be all in one patch?

I'll comment further on github to avoid more spam here.

Spams are welcomed :)
Anyway, it's rather large patch, but there is not much changes included
other
than the RFC if you read the code.

There is createSid() change. I think it's the only one.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi all,

I did this prototype/concept about SessionHandler,
SessionHandlerInterface in PHP 5.6:
https://gist.github.com/narfbg/9643416
In the context of this discussion, the important part is the
updateTimestamp() method (in relation to 'lazy_write'), you should
ignore the rest (for now). I want to discuss it here first before
updating the RFC with it.

So, I had a chat with Yasuo about it to identify the problems and/or
limitations and this is the most important one:

The patch (https://github.com/php/php-src/pull/628) doesn't allow
calling parent::updateTimestamp() and users should call
parent::write() instead. This is because there are PECL extensions
that provide a session handler and they won't have support for that,
which is hard to detect at runtime.
However, I don't think that this is convenient, nor that documenting
this is better than documenting that PECL extensions might not support
the parent::updateTimestamp() call. To me, that's an unnecessary
limitation and here are the alternatives:

Don't declare SessionHandler::updateTimestamp() if
session.save_handler doesn't support it and put a big red warning in
the docs that PECL extensions might lack the method.

I think this is a good trade-off, given that most users trust (or
have) only "stock" PHP extensions and can benefit from this.
Also, this wouldn't be a precedent - you can take mysqli and it's
dependancies on mysqlnd for example (where the docs mention this very
briefly).

Scrap the updateWhatever() method and just modify write() to only
update the timestamp when necessary.

I have no idea why nobody has thought about this previously. The
'lazy_write' idea is inspired by a feature request from 2002
(https://bugs.php.net/bug.php?id=17860) that simply asks for an API
identifying whether $_SESSION has been changed or not, or in other
words - and API saying whether session data or only the timestamp
should be updated.

Now, Yasuo prefers providing explicit API for the lazy_write feature
(namely, the updateTimestamp() method), but simply providing such a
flag via i.e. yet another magic constant, similar to SID, solves ALL
of the problems:

No API changes
No BC breaks
No effect on PECL extensions that don't support it

IMO, that's way more flexible and would allow doing lazy_write by
default, no option - after all, the feature represents a performance
improvement.
It also solves another, side-effect problem: the
SessionUpdateTimestampInterface, that currently exists only because of
internal implementation details.

Thoughts? More suggestions? Concerns?

Cheers,
Andrey.

10 years ago by John LeSueur — view source

unread

Hi all,

I did this prototype/concept about SessionHandler,
SessionHandlerInterface in PHP 5.6:
https://gist.github.com/narfbg/9643416
In the context of this discussion, the important part is the
updateTimestamp() method (in relation to 'lazy_write'), you should
ignore the rest (for now). I want to discuss it here first before
updating the RFC with it.

So, I had a chat with Yasuo about it to identify the problems and/or
limitations and this is the most important one:

The patch (https://github.com/php/php-src/pull/628) doesn't allow
calling parent::updateTimestamp() and users should call
parent::write() instead. This is because there are PECL extensions
that provide a session handler and they won't have support for that,
which is hard to detect at runtime.
However, I don't think that this is convenient, nor that documenting
this is better than documenting that PECL extensions might not support
the parent::updateTimestamp() call. To me, that's an unnecessary
limitation and here are the alternatives:

Don't declare SessionHandler::updateTimestamp() if
session.save_handler doesn't support it and put a big red warning in
the docs that PECL extensions might lack the method.

I think this is a good trade-off, given that most users trust (or
have) only "stock" PHP extensions and can benefit from this.
Also, this wouldn't be a precedent - you can take mysqli and it's
dependancies on mysqlnd for example (where the docs mention this very
briefly).

Scrap the updateWhatever() method and just modify write() to only
update the timestamp when necessary.

I have no idea why nobody has thought about this previously. The
'lazy_write' idea is inspired by a feature request from 2002
(https://bugs.php.net/bug.php?id=17860) that simply asks for an API
identifying whether $_SESSION has been changed or not, or in other
words - and API saying whether session data or only the timestamp
should be updated.

Now, Yasuo prefers providing explicit API for the lazy_write feature
(namely, the updateTimestamp() method), but simply providing such a
flag via i.e. yet another magic constant, similar to SID, solves ALL
of the problems:

No API changes

No BC breaks

No effect on PECL extensions that don't support it

IMO, that's way more flexible and would allow doing lazy_write by
default, no option - after all, the feature represents a performance
improvement.
It also solves another, side-effect problem: the
SessionUpdateTimestampInterface, that currently exists only because of
internal implementation details.

Thoughts? More suggestions? Concerns?

Cheers,
Andrey.

--

Just an unimportant note on the name of the function. Having done this in a
custom session handler, I have always thought of this as an analogue to the
"touch" command for the filesystem. Naming this SessionHandler::touch()
would probably appeal to those familiar with linux or other posix compliant
systems.

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/touch.html

Thanks,
John

10 years ago by Andrey Andreev — view source

unread

Hi,

Just an unimportant note on the name of the function. Having done this in a
custom session handler, I have always thought of this as an analogue to the
"touch" command for the filesystem. Naming this SessionHandler::touch()
would probably appeal to those familiar with linux or other posix compliant
systems.

Yes, probably ... but it would be confusing for those not familiar
with UNIX-based OSes, and especially confusing when you implement a
session handler that is not file-system based.
Anyway, if it gets "merged" into write(), that's irrelevant and this
is the better option, IMO.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

Hi Andrey,

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

"lazy_write" is not accepted. So you don't need to mention it in the RFC.

"read_only" option is better/much faster version of "session_start();
session_commit()".
If the name "read_only" is confusing, better name would be appreciated.

BTW, I have to commit accepted RFC change now. Option name may be changed
before release. "write short cut" is there, since it does not affect
existing behavior
nor save handlers at all.

Oops, Proposal 2 is actually a "write short circuit". Therefore, if you
think this is not
needed, you may mention it in the RFC. I was mixed up with "unsafe_lock"
option, sorry.

I'll make changes required, then merge it soon.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Yasuo Ohgaki — view source

unread

Hi Andrey,

I've finished modifying patch, so I read your RFC.

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

// $_SESSION['logged_in'] has been set in a previous request
Request 1: session_start(['read_only' => TRUE]);
Request 2: session_start(); unset($_SESSION['logged_in']);
session_write_close();
Request 1: if ($_SESSION['logged_in']) { /* logic */ } // evaluates to TRUE

Your example ends up with the same result regardless of "read_only".
If Request 1 locks session data, it finishes execution as logged_in==TRUE.

There may be race condition like this. However, it does not matter much if
legitimate users' requests are processed as authenticated or not under such
race condition. It's legitimate users' request anyway.

If apps must not allow such race condition, any of
session_write_close/commit(), read_only
should not be used. (Request serialization is not perfect still, though.
Please read
session_regenerate_id() thread for details.)

The RFC describes that with 'lazy_write' set to TRUE, if no change was
made to session data, then PS_UPDATE_FUNC() will be called instead of
PS_WRITE_FUNC(). However, it only talks about internal implementation and
fails to address (or at least mention) that a custom session handler should
now also have an update() method.

This is not correct.
User defined save handler does not have to implement update() method.
Session module is designed to allow omitting new method/function just
like undocumented create_sid() function/method. There are test cases
for it. (BTW, I noticed that I missed to compare dummy function address.
It's fixed in my repository already.)

"lazy_write" could be INI option and default to false, but it makes little
sense
because it is compatible with current behavior. I cannot think of good
reason to
disable it, but it may be disabled if you need.

I don't see good reason to postpone this change still.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Andrey Andreev — view source

unread

Hi,

// $_SESSION['logged_in'] has been set in a previous request
Request 1: session_start(['read_only' => TRUE]);
Request 2: session_start(); unset($_SESSION['logged_in']);
session_write_close();
Request 1: if ($_SESSION['logged_in']) { /* logic */ } // evaluates to TRUE

Your example ends up with the same result regardless of "read_only".
If Request 1 locks session data, it finishes execution as logged_in==TRUE.

There may be race condition like this. However, it does not matter much if
legitimate users' requests are processed as authenticated or not under such
race condition. It's legitimate users' request anyway.

Read the explanation in my previous mail:
http://marc.info/?l=php-internals&m=139498773308515

If apps must not allow such race condition, any of
session_write_close/commit(), read_only
should not be used. (Request serialization is not perfect still, though.
Please read
session_regenerate_id() thread for details.)

It's about making a conscious choice of when to use it, not if at all.
The name, and it's presence as an option instead of a separate
function is misleading.

The RFC describes that with 'lazy_write' set to TRUE, if no change was
made to session data, then PS_UPDATE_FUNC() will be called instead of
PS_WRITE_FUNC(). However, it only talks about internal implementation and
fails to address (or at least mention) that a custom session handler should
now also have an update() method.

This is not correct.
User defined save handler does not have to implement update() method.
Session module is designed to allow omitting new method/function just
like undocumented create_sid() function/method. There are test cases
for it. (BTW, I noticed that I missed to compare dummy function address.
It's fixed in my repository already.)

It's not incorrect, the RFC doesn't mention this case and what it
happens in it. I admit, I haven't looked at the patch and the tests,
but if PS_UPDATE_FUNC() doesn't exist, how would you call it?

The session ID is something consumed by (or passed to) the session
handler, there's a big difference.

"lazy_write" could be INI option and default to false, but it makes little
sense
because it is compatible with current behavior. I cannot think of good
reason to
disable it, but it may be disabled if you need.

I don't see good reason to postpone this change still.

Again, nothing explains how that compatibility is maintaned and this
is exactly the concern that I have. When something is unknown, it
usually produces side effects.

Cheers,
Andrey.

10 years ago by Yasuo Ohgaki — view source

unread

I'm announcing the following RFC for discussion, with the hope that it
can get through before the PHP 5.6 release:
https://wiki.php.net/rfc/session-read_only-lazy_write

As noted in it, I don't feel like
https://wiki.php.net/rfc/session-lock-ini was handled properly. Lack
of attention to it alone is demonstrated by the fact that a total of
only 10 people have voted. I hope that this follow-up receives more
attention, so that we can avoid a potential mess.

Because it was designed as a function argument only (and not an INI
setting), there is no API to tell users if/when 'lazy_write' is currently
used.

I understand this is because users don't like INI settings and that there
are way too many of them for sessions already. This is a valid argument of
course, but it completely ignores the fact that INIs exist for a reason and
they are a necessity in some cases. This is one of those cases.

I agree this part. INI is not bad thing.
To illustrate why sticking with INI is good, I coded session_start() as
follows.
If options are INIs, we may eliminate strncmp()s. Longer lines are
truncated, please refer to
https://github.com/yohgaki/php-src/compare/PHP-5.6-rfc-session-lock for
complete patch.

-#define php_session_set_opt_bool(hash, target, key) do {
+#define php_session_start_set_opt_bool(hash, target, key) do {
zval **entry;
if (zend_hash_find(Z_ARRVAL_P(hash), key, sizeof(key), (void
**)&entry) == SUCCESS) {
convert_to_boolean(*entry);
@@ -2276,11 +2277,21 @@ static PHP_FUNCTION(session_decode)
}
} while(0);

+static int php_session_start_set_ini(char *varname, uint varname_len, char
*new_value, uint new_value_len TSR

```
  char buf[128];
```

  snprintf(buf, 127, "%s.%s", "session", varname);

  return zend_alter_ini_entry_ex(buf, strlen(buf)+1, new_value,

new_value_len, PHP_INI_USER, PHP_INI_STA
+}

/* {{{ proto bool session_start(void)
Begin session - reinitializes freezed variables, registers browsers etc
*/
static PHP_FUNCTION(session_start)
{
zval *options = NULL;

```
  zval **value;
```

  HashPosition pos;

  if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "|a",

&options) == FAILURE) {
RETURN_FALSE;
@@ -2288,12 +2299,40 @@ static PHP_FUNCTION(session_start)

    /* set options */
    if (options) {

          /* I'll refactor option handling later. It's not smart. */

```
          PS(read_only) = 0;
```

          php_session_set_opt_bool(options, PS(read_only),

"read_only"); /* this does not have to

          php_session_set_opt_bool(options, PS(lazy_write),

"lazy_write");

```
          /* Iterate through hash */
```

          zend_hash_internal_pointer_reset_ex(Z_ARRVAL_P(options),

&pos);

          while (zend_hash_get_current_data_ex(Z_ARRVAL_P(options),

(void **)&value, &pos) == SUCCESS) {

```
                  char *key;
```
```
                  zend_uint key_len;
```
```
                  ulong index;
```

                  zend_hash_get_current_key_ex(Z_ARRVAL_P(options),

&key, &key_len, &index, 0, &pos);

                  switch(Z_TYPE_PP(value)) {

                          case IS_ARRAY:

                          case IS_DOUBLE:

                          case IS_OBJECT:

                          case IS_NULL:

                          case IS_RESOURCE:

                          case IS_CALLABLE:

                                  php_error_docref(NULL TSRMLS_CC,

E_WARNING, "Option(%s) value must be

                                  break;

```
                          default:
```

                                  if (!strncmp(key, "read_only",

sizeof("read_only")-1)) {

                                          PS(read_only) = 0;

php_session_start_set_opt_bool(options, PS(read_only), "

                                  } else if (!strncmp(key,

"lazy_write", sizeof("lazy_write")-1)) {

php_session_start_set_opt_bool(options, PS(lazy_write), "

                                  } else {

                                          convert_to_string(*value);

if

(php_session_start_set_ini(key, key_len, Z_STRVAL_PP(value)

php_error_docref(NULL TSRMLS_CC, E_WARNING, "Setting o

```
                                  }
```

                                  break;

```
                  }
```

                  zend_hash_move_forward_ex(Z_ARRVAL_P(options),

&pos);

```
          }
  }
```

  /* allow to set INI options? */

  php_session_start(TSRMLS_C);

This could be optimized/cleaned by using ini modifying handler function
hash. In return, there
would be number of similar functions. (I'm not sure which one is faster)

Using INI could make code a little simpler.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

10 years ago by Stas Malyshev — view source

unread

Hi!

If options are INIs, we may eliminate strncmp()s. Longer lines are
truncated, please refer to
https://github.com/yohgaki/php-src/compare/PHP-5.6-rfc-session-lock for
complete patch.

Could you make a pull request? It is impossible on GH to comment on
compare results.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227