Hi all,
New session feature has been discussed while.
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-ini
New comments and questions are welcomed.
Thank you for voting.
P.S. I have removed session_discard(). I've committed
it as session_abort() to address FR last year. Name/alias
is open to be changed. Please comment on different thread
for this.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-iniNew comments and questions are welcomed.
I see there are three proposals there, but only one vote. E.g. I think
unlocked sessions are not a good idea, but lazy write may be ok (btw,
interaction between no-lock and lazy write can lead to completely
undebuggable scenarios when same data by same code sometimes are written
and sometimes are not depending on session state). I'm not sure I even
understand what lazy_destroy does - if the data is deleted then it's
deleted, how can it be accessible and why?
Also, do I understand correctly that the options would apply only to
files module? If so, why they under session.* and not session.files.* or
something to emphasize that if you're not using files module it doesn't
do anything?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Mon, Jan 20, 2014 at 11:56 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-iniNew comments and questions are welcomed.
I see there are three proposals there, but only one vote. E.g. I think
unlocked sessions are not a good idea, but lazy write may be ok (btw,
interaction between no-lock and lazy write can lead to completely
undebuggable scenarios when same data by same code sometimes are written
and sometimes are not depending on session state).
It is made a single RFC so that everyone understand new features.
I agree that unlocked session could be a cause of hard to debug bugs and
it should never be a default.
Even if unlocked session can be dangerous, there are save handlers
that support unlocked session for better performance. e.g. memcache. It
would
be reasonable to support by module.
Unlocked session could be more usable by session.lazy_write. According to
current
implementation, "the last session wins". With session.lazy_write, "the last
session
writes data wins". This behavior prevents loosing updated data. It's
better for
performance also.
I'm not sure I even
understand what lazy_destroy does - if the data is deleted then it's
deleted, how can it be accessible and why?
With AJAX or browser supports concurrent access to server,
concurrent access to server is possible.
When session ID is regenerated, it is possible that some connections access
to
server with old session ID. i.e. Race condition.
When this happened, old session that may be known to attacker may be
reinitialized or new unneeded session ID is created when use_strict_mode=On.
Allowing access to old session data for a while prevents these cases that
initialize
unneeded session.
RFC proposes deletion flag in $_SESSION. It's a little dirty,
but it's faster and simpler. It would be acceptable because the session is
deleted one.
Also, do I understand correctly that the options would apply only to
files module? If so, why they under session.* and not session.files.* or
something to emphasize that if you're not using files module it doesn't
do anything?
No. It's applicable to all save handlers.
files save handler is default save handler and there were comments that
want separate implementation for files. That's the reason why there is
such
voting option.
files and files_ext would be identical code except the code supports
proposed options. I would like to have single code for easier maintenace.
IMHO.
Thank you for the comment.
I hope everyone understands idea behind this RFC.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
With AJAX or browser supports concurrent access to server,
concurrent access to server is possible.When session ID is regenerated, it is possible that some connections
access to
server with old session ID. i.e. Race condition.When this happened, old session that may be known to attacker may be
reinitialized or new unneeded session ID is created when
use_strict_mode=On.Allowing access to old session data for a while prevents these cases that
initialize
unneeded session.RFC proposes deletion flag in $_SESSION. It's a little dirty,
but it's faster and simpler. It would be acceptable because the session is
deleted one.
Additional comment for this.
session_destroy()/session_gc() deletes session data immediately.
session.lazy_destroy is applicable only when session_regenerate_id() is
called.
The name (session.lazy_destroy) might be better to be changed or
it might be better apply for session_destroy() since there would be
similar race condition like session_regenerate_id().
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
Unlocked session could be more usable by session.lazy_write. According
to current implementation, "the last session wins". With session.lazy_write, "the
last session writes data wins". This behavior prevents loosing updated data. It's
better for performance also.
I think it's not a good idea. This creates a setup where you never know
if your action on session state would actually result in any action at
all. It would be as good for performance as a database that skips random
data writes. Performance may be great, but nobody would use such
database because it's unreliable. I don't think we should put into PHP a
mechanism that sets up users for trouble.
With AJAX or browser supports concurrent access to server,
concurrent access to server is possible.When session ID is regenerated, it is possible that some connections
access to
server with old session ID. i.e. Race condition.
I don't see how it is possible. I.e. it can access with the old ID, but
if it happened before the session was deleted, then it gets the session,
if it happens after, it gets "no session". Isn't that why you proposed
strict sessions? It of course is also possible to handle without it -
request after deletion would get empty session state and would have to
return error message and have the client to re-request with correct
session state.
Do you mean that somebody would regenerate the id but not destroy the
old session? I don't see how it would be useful - if you have any
security context, the whole point is to destroy the old session, why
won't you do that?
When this happened, old session that may be known to attacker may be
reinitialized or new unneeded session ID is created when use_strict_mode=On.
It of course may be reinitialized, but what's wrong with that? Deleted
data would not be accessible anyway - the data is already gone.
Allowing access to old session data for a while prevents these cases
that initialize unneeded session.
That would be a security hole - the system thinks the user was logged
out but you allow anybody who still has old session ID still access the
session as if nothing happened? That's not good at all.
No. It's applicable to all save handlers.
Then what's the point of proposing additional handler if it applies to
all handlers anyway?
files save handler is default save handler and there were comments that
want separate implementation for files. That's the reason why there is such
voting option.files and files_ext would be identical code except the code supports
proposed options. I would like to have single code for easier
maintenace. IMHO.
I don't understand. If all save handlers have this option, what is the
point of existence of both files and files_ext? And what about other
handlers - do they also have to have _ext versions?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Mon, Jan 20, 2014 at 2:04 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Unlocked session could be more usable by session.lazy_write. According
to current implementation, "the last session wins". With
session.lazy_write, "the
last session writes data wins". This behavior prevents loosing updated
data. It's
better for performance also.I think it's not a good idea. This creates a setup where you never know
if your action on session state would actually result in any action at
all. It would be as good for performance as a database that skips random
data writes. Performance may be great, but nobody would use such
database because it's unreliable. I don't think we should put into PHP a
mechanism that sets up users for trouble.
I agree that users could get into trouble if they don't know what they are
doing.
To get most out of session.lock=off and session.lazy_write=on, proper usage
is needed to get reliable operation
Suppose session only stores authentication info, then session and data
storage
only needs read. Session write (or destroy) is only needed when users are
logged
out. Session write is not needed almost all. PHP could cut session and
storage
load to half and PHP could allow concurrent access for apps while app
could
maintain consistency perfectly.
In this scenario, app performance could be boosted a lot by removed write()
calls.
Programmer also don't have to care to call
session_commit()/session_write_close() to
get concurrency. Session module writes session data when it is needed.
(Programmers
must be careful about concurrent write() calls, though)
At least I'll use this and there would be certain number of users who use
like this
for better performance.
Users, who want correct access counts or like, should not use
session.lock=off.
session.lazy_write=on will not give any benefits if program updates
$_SESSION
for every request.
With AJAX or browser supports concurrent access to server,
concurrent access to server is possible.
When session ID is regenerated, it is possible that some connections
access to
server with old session ID. i.e. Race condition.I don't see how it is possible. I.e. it can access with the old ID, but
if it happened before the session was deleted, then it gets the session,
if it happens after, it gets "no session". Isn't that why you proposed
strict sessions?
Strict session is to prevent session to initialize with user supplied
session ID.
In this case, session ID is initialized by PHP and I would like to allow
access to
deleted session for a while to prevent race condition that creates unneeded
new session ID (and it's data).
It of course is also possible to handle without it -
request after deletion would get empty session state and would have to
return error message and have the client to re-request with correct
session state.
Current session module implementation and web technology allows to access
old session ID when session ID is regenerated. As you described, session X
could
remain deleted.
Request A Request B
↓ |
Access session X ↓
| Access session X (if
locked, Req. B waits )
↓ ↓
Regenerate ID (Del X, New Y) session X unlocked (Del X unlocks)
↓ ↓
Close session Y Close session X (For files, inode is
marked deleted remain deleted)
However, this could happen also.
Request A Request B
↓ |
Access session X |
↓ |
Regenerate ID (Del X, New Y) |
↓ |
Close session Y |
Access session X
↓
Session X is created
(use_strict_mode=on creates session Z)
↓
Close session X (Session X
or Z remains)
Without strict session, old session X could remain. With strict session,
unneeded Z could remain.
Even if programmers want to delete session X, session X could be
reinitialized by default config.
There are more cases with session.lock=off.
Do you mean that somebody would regenerate the id but not destroy the
old session? I don't see how it would be useful - if you have any
security context, the whole point is to destroy the old session, why
won't you do that?
It's described in previous.
Users wouldn't, but system might do.
When this happened, old session that may be known to attacker may be
reinitialized or new unneeded session ID is created when
use_strict_mode=On.It of course may be reinitialized, but what's wrong with that? Deleted
data would not be accessible anyway - the data is already gone.
I have to double check, but for example, current files do not wipe data
file content. It's a simple unlink. I suppose it has old data. It will be
inaccessible
eventually as described, though.
For database based session, reading deleted data should be result in error.
(e.g. Serializable transaction isolation level is needed for this)
Allowing access to old session data for a while prevents these cases
that initialize unneeded session.That would be a security hole - the system thinks the user was logged
out but you allow anybody who still has old session ID still access the
session as if nothing happened? That's not good at all.
It allows access to old session for certain amount of time. 30 seconds
would be enough for session.lock=off. 60 or more when session.lock=on.
I was about to allow access to deleted session for 90 seconds.
Configurable INI may be better. I'll make session.lazy_destroy a integer
config.
No. It's applicable to all save handlers.
Then what's the point of proposing additional handler if it applies to
all handlers anyway?
files may simply ignore new INI config since there may not be codes support
new options.
files_ext is "files + new INI config support".
IIRC you're one of them who are preferred to have separate
implementation(?)
I don't mind at all to remove files_ext proposal.
files save handler is default save handler and there were comments that
want separate implementation for files. That's the reason why there is
such
voting option.files and files_ext would be identical code except the code supports
proposed options. I would like to have single code for easier
maintenace. IMHO.I don't understand. If all save handlers have this option, what is the
point of existence of both files and files_ext? And what about other
handlers - do they also have to have _ext versions?
No, they don't have to. They can ignore new features or implement them
all or part of them.
I would like to know session save handler capability. I'll add save handler
API
that declares it's capability.
I'll update the RFC later.
Thank you for your comment. The RFC is getting better.
I understand your anxiety. I'm not going to change current behavior,
just adding new features for advanced save handlers and users.
I'll write big caution to the manual also.
Shooting their own foot is possible, but let advanced users to enjoy
extreme performance!
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
It allows access to old session for certain amount of time. 30 seconds
would be enough for session.lock=off. 60 or more when session.lock=on.
I was about to allow access to deleted session for 90 seconds.
Configurable INI may be better. I'll make session.lazy_destroy a integer
config.
It is irrelevant how long it is. Even 1 microsecond is long enough - if
the app said session data should be gone, then they should be gone, not
linger around and allow access to them. If you need access to that data,
copy it into the new session.
IIRC you're one of them who are preferred to have separate
implementation(?)
I prefer not to have implementation for 2 out of 3 proposed "features"
at all, because I think they will produce more problems than solutions.
For the lazy-write feature, IMO it can be made global, or function (I
actually prefer both) - but I see no reason to create a duplicate
driver, if you don't want it turned on, turn off the option. I'm not
sure what would be the reason to clone the driver. Do you have a
reference for where it was asked so I could re-read it and recall what
were the reasons for it? My memory may be failing me :)
No, they don't have to. They can ignore new features or implement them
all or part of them.
This would be a bit confusing - you set an option, but some drivers
would implement it, some would ignore it, so you never know what
actually happens.
Shooting their own foot is possible, but let advanced users to enjoy
extreme performance!
I don't think performance at he expense of reliability, data consistency
and security should be our goal. Like I said, we could also make an
option to ignore random number of file writes and claim it is a
performance feature, but it would be useless, since there is no case
where one would recommend to use it. I see no case where I would
recommend anybody to use unlocked sessions - so why introduce it?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas
On Mon, Jan 20, 2014 at 4:35 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
It allows access to old session for certain amount of time. 30 seconds
would be enough for session.lock=off. 60 or more when session.lock=on.
I was about to allow access to deleted session for 90 seconds.
Configurable INI may be better. I'll make session.lazy_destroy a integer
config.It is irrelevant how long it is. Even 1 microsecond is long enough - if
the app said session data should be gone, then they should be gone, not
linger around and allow access to them. If you need access to that data,
copy it into the new session.
I agree. If app delete it now, it should be delete immediately.
It is possible with session.lazy_destory=0.
Since server and client is not synchronized, there are edge case that
app may allow to access old session for a while. It should be allowed. IMO.
IIRC you're one of them who are preferred to have separate
implementation(?)I prefer not to have implementation for 2 out of 3 proposed "features"
at all, because I think they will produce more problems than solutions.
For the lazy-write feature, IMO it can be made global, or function (I
actually prefer both) - but I see no reason to create a duplicate
driver, if you don't want it turned on, turn off the option. I'm not
sure what would be the reason to clone the driver. Do you have a
reference for where it was asked so I could re-read it and recall what
were the reasons for it? My memory may be failing me :)
No, they don't have to. They can ignore new features or implement them
all or part of them.This would be a bit confusing - you set an option, but some drivers
would implement it, some would ignore it, so you never know what
actually happens.
That's the reason why I would like to add new API to declare save handler
capability. If I know capability, I may assume what it does.
Shooting their own foot is possible, but let advanced users to enjoy
extreme performance!I don't think performance at he expense of reliability, data consistency
and security should be our goal. Like I said, we could also make an
option to ignore random number of file writes and claim it is a
performance feature, but it would be useless, since there is no case
where one would recommend to use it. I see no case where I would
recommend anybody to use unlocked sessions - so why introduce it?
In previous mail, it is described good use case.
Without locking, scripts are executed concurrently.
When session.lock=off and session.lazy_write=on, scripts does not have
to wait session read() and only updated session is written back to storage.
For example, if session only store authentication information, it works
perfectly and there is no risk of authenticated(logged out session) remains
or un-authenticated session became an authenticated session.
We also should consider that memcache/memched have unlock option already.
Like databases allow query without transaction or transaction with less
consistency, session module may allow options that give more performance
at the cost of consistency. It's an option for developers. IMHO.
(Current session is the same as "serializable transaction isolation level"
only
database which is very slow and restrict concurrency.)
I'll write clearly that "end users" should never enable dangerous options
unless
developer explicitly allows it. I'll also write that developers are better
to detect
dangerous options if app does not support them. Is it enough, isn't it?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
In previous mail, it is described good use case.
Without locking, scripts are executed concurrently.
If the scripts are going to write the session, they can not execute
concurrently, because that creates race condition and at least one of
the states will be lost. If they don't intend to write, they can just
drop the lock immediately after reading, that's why we talked about
having function for that. Neither works well with no locks at all.
When session.lock=off and session.lazy_write=on, scripts does not have
to wait session read() and only updated session is written back to storage.
For example, if session only store authentication information, it works
perfectly and there is no risk of authenticated(logged out session) remains
It doesn't work perfectly for scripts that modify authentication
information. And writing an app where there are race conditions in
authentication mechanism is very poor idea, security-wise. For
performance in read-only case, there's a simple solution which does not
jeopardize stability, see above.
We also should consider that memcache/memched have unlock option already.
I would not recommend anybody then to use them with that option. This
only can lead to trouble. It's running shared data structure without any
locks. If it's not specially designed for it, you'll get in trouble -
and it will always be undebuggable, unreproducible and happen in the
worst possible moment, when the system is loaded and too many things are
happening at once.
Like databases allow query without transaction or transaction with less
If you are using DB without transactions, you must ensure consistent
state in your app, or accept data inconsistency. Data inconsistency may
be ok if your data is not that important - but if it's
security-sensitive, it is not a good idea. Sessions usually deal with
security-sensitive data.
I'll write clearly that "end users" should never enable dangerous
options unless
developer explicitly allows it. I'll also write that developers are
I don't think we should provide options that need disclaimers that say
"do not use it unless in very rare cases". If you are experienced enough
that you need this rare case, you can implement your own session handler
that does that. Especially given that the only use case I have seen so
far actually does not need this feature and works just fine with much
safer and smaller feature (session unlock, aka session_discard, aka
session_abort).
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Mon, Jan 20, 2014 at 5:50 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
I'll write clearly that "end users" should never enable dangerous
options unless
developer explicitly allows it. I'll also write that developers areI don't think we should provide options that need disclaimers that say
"do not use it unless in very rare cases". If you are experienced enough
that you need this rare case, you can implement your own session handler
that does that. Especially given that the only use case I have seen so
far actually does not need this feature and works just fine with much
safer and smaller feature (session unlock, aka session_discard, aka
session_abort).
This is feasible option.
There is session_abort() already. It's nice to have session_unlock().
I'll add session_unlock(). It requires new save handler API, but API is
modified anyway.
I understand your discussion and anxiety. People do stupid things w/o
knowing what they are doing.
Yet, I would like to have "non transaction query" like session data
handling, since there are apps that work as it should without
additional/modified code and enjoy much better performance.
I'll change voting option so that people could vote feature by feature.
There are 3 users including me who vote to this RFC. They have to
vote again.
I'll send mail when I finish editing the RFC, please hold to vote.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
I'll send mail when I finish editing the RFC, please hold to vote.
Before I re-open vote, I would like to ensure if the RFC has no
issues to discuss.
https://wiki.php.net/rfc/session-lock-ini
Thank you for reviewing RFC!
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Stas,
I'll send mail when I finish editing the RFC, please hold to vote.
Before I re-open vote, I would like to ensure if the RFC has no
issues to discuss.https://wiki.php.net/rfc/session-lock-ini
Thank you for reviewing RFC!
I was forgetting whole point of having session.lazy_destroy.
This feature should be enabled by default to destroy old session
when session_regenerate_id() is called.
Regenerating session ID with reliable manner is mandatory for
better security. Therefore, I've changed RFC to enable session.lazy_destroy
by default. Default delay is matter to discuss, though.
Please refer to the referenced discussion in the RFC.
http://marc.info/?l=php-internals&m=138242492914526&w=2
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
op 20-01-14 01:37, Yasuo Ohgaki schreef:
Hi all,
New session feature has been discussed while.
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-iniNew comments and questions are welcomed.
It's funny you mention the memcache session save handler. I remember
when it was introduced quite some people were scratching their heads
about unexpected session data behavior. Nothing in the module
documentation even talked about it not locking the session data by
default, therefore last script exiting writing it's version of the
session data (where a script not changing the session data would write
back the old session state removing other scripts' changes as it was
just last to end. so much fun debugging non consistent session changes
as a result of scripts ending in a different order).
I thought it was really bad behavior and complained about it then. The
things you try to solve (concurrent access / speeding up performance)
can for the most part already be done with the current session logic in
a well defined consistent manner. opening and locking the session,
reading and writing the needed session variables, closing and unlocking
the session with session_write_close and continuing script execution
after which other scripts have access to the session. For many
programmers a familiar pattern similar to critical sections and mutexes
in threaded environments.
I see only one very simple function potentially being helpful. A simple
session close without write maybe called session_close(). This would
give a performance bonus as it will not need to serialize the session
data and write it back to session storage. It also helps to programmer
in cases that he wants to guarantee that the session store is not
changed by script (similar to a transaction abort).
just my $.02
Bas van Beek
Thank you for voting.
P.S. I have removed session_discard(). I've committed
it assession_abort()to address FR last year. Name/alias
is open to be changed. Please comment on different thread
for this.--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Bas,
op 20-01-14 01:37, Yasuo Ohgaki schreef:
Hi all,
New session feature has been discussed while.
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-iniNew comments and questions are welcomed.
It's funny you mention the memcache session save handler.
Are they removed or I remembered incorrectly.
This is taken from memcached 2.2.0b1
php_memcached.c: STD_PHP_INI_ENTRY("memcached.sess_locking", "1", PHP_INI_ALL,
OnUpdateBool, sess_locking_enabled, zend_php_memcached_globals,
php_memcached_globals)
I should update RFC.
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Bas,
I see only one very simple function potentially being helpful. A simple
session close without write maybe called session_close(). This would give
a performance bonus as it will not need to serialize the session data and
write it back to session storage. It also helps to programmer in cases
that he wants to guarantee that the session store is not changed by
script (similar to a transaction abort).
I have already added this as session_abort().
Since it's only in 5.6 and master branch, the name could be changed.
However, session_abort() (or something like session_discard()) would be
better name because the name implies it does not save data.
If you come up with better name, please let me know.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Bas,
I see only one very simple function potentially being helpful. A simple
session close without write maybe called session_close(). This would give
a performance bonus as it will not need to serialize the session data and
write it back to session storage. It also helps to programmer in cases
that he wants to guarantee that the session store is not changed by
script (similar to a transaction abort).I have already added this as
session_abort().
Since it's only in 5.6 and master branch, the name could be changed.
However,session_abort()(or something like session_discard()) would be
better name because the name implies it does not save data.If you come up with better name, please let me know.
I think those two are useful:
-
session_abort()- close session, do not write, unlock file - session_close() - close session, do write, unlock file
I do think this also should be application defined, not configuration
defined. So using functions for that is better than ini options. Only
the individual script nows what it needs, global configuration for
tuning one application will break another on the same system and makes
it harder to write portable appliations.
johannes
Hi Johanns and Stas,
- session_close() - close session, do write, unlock file
We have session_write_close() (session_commit() is alias), so
it may be better to remain as it is.
On Mon, Jan 20, 2014 at 11:18 PM, Johannes Schlüter
johannes@schlueters.dewrote:
I do think this also should be application defined, not configuration
defined. So using functions for that is better than ini options. Only
the individual script nows what it needs, global configuration for
tuning one application will break another on the same system and makes
it harder to write portable appliations.
Stas and your discussion sounds reasonable to me, too. I don't mind
files_ext implementation to be a PECL module as a reference implementation.
It would be a module that does not provide any function, but only a
files_ext
save handler and lock INI.
session_unlock() is a simple new function and it does not require RFC, but
it is mentioned in it. However, unlocking session after start makes thing
more
complex for save handlers. It is better to specify when session is started.
Therefore, it would be better to pass new behavior options to
session_start().
session_start(array('lock'=>false, 'lazy_write'=>true,
'lazy_destroy'=>120));
It may also set other options for session like save handler, serializer,
expire, etc.
Since session.lazy_destroy is required for reliable session_regenerate_id()
operation, it remains but not exposed as INI option. Users has to pass
option
to session_start() to disable it.
In short, no new INI is added. Option parameter is added to session_start().
I'll add new API for save handler to get start options.
I would like to change the RFC like this.
Any comments?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
I would like to change the RFC like this.
Any comments?
I've updated the RFC.
https://wiki.php.net/rfc/session-lock-ini
Removed INI options and options are session_start() parameter now.
It has changed to satisfy most comments hopefully.
Could you review the RFC before reopen vote?
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
https://wiki.php.net/rfc/session-lock-ini
Before I re-open vote of this RFC, I would like to add 'read_only'
parameter for
just reading session and close immediately. This was suggested by Bill
Salak. This option is useful and the fastest way to read session.
I updated the RFC.
There are 6 vote options now. It would be too much.
Does anyone mind if I make the RFC a single vote?
Or should I separate Proposal 2 (minimize_lock)?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
There are 6 vote options now. It would be too much.
Does anyone mind if I make the RFC a single vote?
Or should I separate Proposal 2 (minimize_lock)?
6 options seems too much. 0 shouln't even be a vote - if people want
specific options, then ipso facto they want them introduced, and
parameter is a way to do it, if none is accepted - there's no reason to
introduce the parameter. I would rather make it part of the whole
proposal and propose both INI value and session_start options (with the
latter overriding the former).
Proposal 5 seems to have nothing to do with the rest. So it probably
needs to be split out.
I'd just leave it as two proposals (this is my personal preference):
- Options & read_only + lazy_write (these ones seem to be least
controversial ones and don't seem to have any negative implications) - lazy_destroy
- minimize_lock if you really want it though I still think it is
dangerous and is not needed if read_only and lazy-write exist and if you
still need it you're doing sessions wrong. But if you really think
there's a case then let's put it as separate one.
So we'd have 2 or 3 options, which I think is the max for vote to be
manageable.
Some more things:
- session_unlock - I don't see how it's different from session_abort.
What's the need for it? - session_module_feature - what is the use case for it? I'm not sure I
understand why anybody would use it. Especially if outputs would be
non-machine-friendly as "Partially Supported" - what would you do with
that? What Partially means here?
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi Stas,
On Thu, Jan 30, 2014 at 10:09 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
6 options seems too much. 0 shouln't even be a vote - if people want
specific options, then ipso facto they want them introduced, and
parameter is a way to do it, if none is accepted - there's no reason to
introduce the parameter. I would rather make it part of the whole
proposal and propose both INI value and session_start options (with the
latter overriding the former).Proposal 5 seems to have nothing to do with the rest. So it probably
needs to be split out.I'd just leave it as two proposals (this is my personal preference):
- Options & read_only + lazy_write (these ones seem to be least
controversial ones and don't seem to have any negative implications)- lazy_destroy
- minimize_lock if you really want it though I still think it is
dangerous and is not needed if read_only and lazy-write exist and if you
still need it you're doing sessions wrong. But if you really think
there's a case then let's put it as separate one.So we'd have 2 or 3 options, which I think is the max for vote to be
manageable.
Sounds reasonable. I'll update RFC according to your comment.
minimize_lock is still useful for users would like to write only when
$_SESSION
is updated. Without it, session data is locked and scripts are not executed
in
parallel. Programmer has to selectively start session read only and
re-start
session for writing when it's needed. Efficient session management could be
done as I wrote/you think, so it's not strictly required.
'minimize_lock' make things easier (no selective read only session nor
re-starting session for writing) for experts when consistency is under
control.
Some more things:
- session_unlock - I don't see how it's different from session_abort.
What's the need for it?
I should have change this description.
session_abort() closes session without writing data.
session_unlock() unlock session data. Data is written when
session_commit()/etc.
session_unlock() is not needed if there is 'minimize_lock'. Unlike
'minimize_lock',
it will slow down performance. It will not be implemented.
- session_module_feature - what is the use case for it? I'm not sure I
understand why anybody would use it. Especially if outputs would be
non-machine-friendly as "Partially Supported" - what would you do with
that? What Partially means here?
This is informational function for programmers, not for programs.
It requires active session to be useful because it needs active save handler
module. It's rather suitable as phpinfo(), but it might not be useful for
users.
Since
<?php
session_module('foo');
// or define/load user save handler and set it
session_start();
phpinfo()
?>
is required to be useful. It may return result as constants. I'm not sure
if it is useful. If anyone insists, I'll change it return constants.
Meaning of "Partially Supported" depends on save handler. It should be
documented in save handler manual what does it mean. For example,
session_gc() with file handler works for single directory, but it's not for
multiple directory. We had number of not-a-bug reports for this.
I would like to reopen vote next week.
Anyone who has comments, please send them now.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hello Yasuo,
First of all, the github "compare" link you give in the RFC shows a vast
amount of unrelated changed files. Playing around a bit I find that comparing
not to php:master but to yohgaki:PHP-5.6 gives a better overview:
https://github.com/yohgaki/php-src/compare/PHP-5.6...PHP-5.6-rfc-session-lock
Regarding minimize_lock:
Generally a really dangerous feature. minimize_lock sounds so friendly, does
not imply danger really.
Alternative naming proposal: unlocked_thus_unsafe
The minimize_lock=true implementation in mod_files.c looks wrong:
-
in PS_READ_FUNC the flock(LOCK_EX) is placed after
fstat(). A concurrent
writer could rewrite the file between the fstat and the pread/read, resulting
in either a short read or an incomplete read, in both cases resulting in
invalid session data being read. -
in PS_WRITE_FUNC the flock(LOCK_EX) is placed after
ftruncate(). Thus, an
ftruncate could hit both concurrent readers and writers at any time, resulting
in various kinds of problems. -
again in PS_WRITE_FUNC in the check whether to ftruncate, data->st_size,
which was set in an earlier PS_READ_FUNC, is used for the comparison. However,
the file on disk may have been changed by a concurrent writer in the meantime,
and have an arbitrary different size, resulting in a wrong decision to
ftruncate-or-not.
To solve all three problems, I'd suggest
- unconditional taking of the flock(LOCK_EX) in ps_files_open(), while keeping
theLOCK_UNin PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true). - also when minimize_lock=true, make an
fstat()call in PS_WRITE_FUNC before
the need-to-ftruncate check, ignoring data->st_size from the read.
Reading that code, also in the base PHP repository, I think I spotted an
unrelated bug: In ps_files_open() in the not-WIN32-open_basedir fstat/S_ISLNK
error cases: data->fd is closed, but data->fd is then NOT set to -1.
best regards
Patrick
To solve all three problems, I'd suggest
- unconditional taking of the flock(LOCK_EX) in ps_files_open(), while
keeping
theLOCK_UNin PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true).
Clarification: unconditionally in case of minimize_lock=true means taking
the LOCK_EX after the huge if, right at the end of ps_files_open, i.e. even
when data->fd was already >= 0. Otherwise the write case would miss the
lock.
If that seems strange in ps_files_open then the LOCK_EX could be placed in
PS_READ_FUNC and PS_WRITE_FUNC directly after successful call to
ps_files_open - doing it that way would have the advantage of making a
local code review in each of the functions self-contained wrt the locking.
best regards
Patrick
Unrelated to the previous discussion, and applicable to the base PHP code
in mod_files.c too, another observation:
Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the
already-locked case the second call will go to sleep for a while waiting
for the lock to clear. But it is also possible, I think, for the
pread/read/write calls. The usual handling for the case (-1 return && errno
== EINTR) is to just repeat the call in a while loop. I think that at least
doing so for the LOCK_EX calls, would be prudent.
best regards
Patrick
Hi Patrick,
Unrelated to the previous discussion, and applicable to the base PHP code
in mod_files.c too, another observation:Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the
already-locked case the second call will go to sleep for a while waiting
for the lock to clear. But it is also possible, I think, for the
pread/read/write calls. The usual handling for the case (-1 return && errno
== EINTR) is to just repeat the call in a while loop. I think that at least
doing so for theLOCK_EXcalls, would be prudent.best regards
I also notice other issue when closely looking at current code.
It does
open()
↓
read() - locked
↓
script execution
↓
write() - unlock
↓
close()
However, when write() is called, it always close opened fd to address
changed session ID.
This logic will likely to create race condition that reads old session data
before writing.
I'll address this issue as other issue to be fixed.
In short, session data access is not serialized like serializable
transaction isolation level.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Yasuo,
On Thursday 30 January 2014 16:17:58 Yasuo Ohgaki wrote:
I also notice other issue when closely looking at current code.
...However, when write() is called, it always close opened fd to address
changed session ID.
I don't see that. You mean by the call to ps_files_close() within
ps_files_open(), and that close() call dropping the lock? That is conditional
on either data->fd < 0 (no file open, just a superfluous call to
ps_file_close()), or key != lastkey (and then a different file will be
opened and locked for writing).
best regards
Patrick
Hi Patrick,
I don't see that. You mean by the call to ps_files_close() within
ps_files_open(), and that close() call dropping the lock? That is
conditional
on either data->fd < 0 (no file open, just a superfluous call to
ps_file_close()), or key != lastkey (and then a different file will be
opened and locked for writing).
Next request may read closed session data which is no longer valid.
When it is destroyed or regenerated session with new code, it will be
handled by
lazy_destroy. So it's not a problem.
When session is not destroyed or regenerated, close call in PS_WRITE
unlocks
file then next request starts reading it. This could happen before writing
data by
previous request.
I don't have access to the code right now, so I have to double check though.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
Unrelated to the previous discussion, and applicable to the base PHP code
in mod_files.c too, another observation:Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the
already-locked case the second call will go to sleep for a while waiting
for the lock to clear. But it is also possible, I think, for the
pread/read/write calls. The usual handling for the case (-1 return && errno
== EINTR) is to just repeat the call in a while loop. I think that at least
doing so for theLOCK_EXcalls, would be prudent.best regards
I also notice other issue when closely looking at current code.
It doesopen()
↓
read() - locked
↓
script execution
↓
write() - unlock
↓
close()However, when write() is called, it always close opened fd to address
changed session ID.
This logic will likely to create race condition that reads old session
data before writing.I'll address this issue as other issue to be fixed.
In short, session data access is not serialized like serializable
transaction isolation level.
It seems we have to keep all opened fd until session is really closed.
It requires rather large change. I'll fix this issue when this RFC is done.
Is anyone has better idea rather than keeping all fds?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the
already-locked case the second call will go to sleep for a while waiting
for the lock to clear. But it is also possible, I think, for the
pread/read/write calls. The usual handling for the case (-1 return &&
errno
== EINTR) is to just repeat the call in a while loop. I think that at
least
doing so for theLOCK_EXcalls, would be prudent.
Re-Read the manpage (*) a bit... read/pread/write should be safe here, going
to disk, and not a pipe or socket. But flock() will return EINTR when
signalled and the signal handler did not use SA_RESTART. Most PHP execution
will probably okay anyway as $restart_syscalls is default true for
pcntl_signal(), but handling would be safer anyway.
Patrick
(*) search for SA_RESTART in http://man7.org/linux/man-pages/man7/signal.7.html
Hi Patrick,
Unrelated to the previous discussion, and applicable to the base PHP code
in mod_files.c too, another observation:Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the already-
locked case the second call will go to sleep for a while waiting for the
lock to clear. But it is also possible, I think, for the pread/read/write
calls. The usual handling for the case (-1 return && errno == EINTR) is to
just repeat the call in a while loop. I think that at least doing so for
theLOCK_EXcalls, would be prudent.
Could you send bug report for this and assign me?
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hello Yasuo,
On Saturday 01 February 2014 10:13:42 Yasuo Ohgaki wrote:
Hi Patrick,
Unrelated to the previous discussion, and applicable to the base PHP code
in mod_files.c too, another observation:Some syscalls can fail with EINTR, when a signal hits while within the
kernel. This is especially true for flock(LOCK_EX) because in the already-
locked case the second call will go to sleep for a while waiting for the
lock to clear. But it is also possible, I think, for the pread/read/write
calls. The usual handling for the case (-1 return && errno == EINTR) is to
just repeat the call in a while loop. I think that at least doing so for
theLOCK_EXcalls, would be prudent.Could you send bug report for this and assign me?
Created https://bugs.php.net/bug.php?id=66623
Don't know how to assign that to you.
Thanks for taking care.
Patrick
Hi Patrick,
First of all, the github "compare" link you give in the RFC shows a vast
amount of unrelated changed files. Playing around a bit I find that
comparing
not to php:master but to yohgaki:PHP-5.6 gives a better overview:https://github.com/yohgaki/php-src/compare/PHP-5.6...PHP-5.6-rfc-session-lock
Regarding minimize_lock:
Generally a really dangerous feature. minimize_lock sounds so friendly,
does
not imply danger really.Alternative naming proposal: unlocked_thus_unsafe
It sounds good idea. How about shorter name?
unsafe_lock
To solve all three problems, I'd suggest
- unconditional taking of the flock(LOCK_EX) in ps_files_open(), while
keeping
theLOCK_UNin PS_READ_FUNC/PS_WRITE_FUNC (if minimize_lock=true).
This would be safer and good for reference implementation.
- also when minimize_lock=true, make an
fstat()call in PS_WRITE_FUNC
before
the need-to-ftruncate check, ignoring data->st_size from the read.
I'll fix this later in my branch.
Reading that code, also in the base PHP repository, I think I spotted an
unrelated bug: In ps_files_open() in the not-WIN32-open_basedir
fstat/S_ISLNK
error cases: data->fd is closed, but data->fd is then NOT set to -1.
This should be set to -1.
I've committed the fix.
Thank you.
Regards.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
On Thursday 30 January 2014 14:34:35 Yasuo Ohgaki wrote:
Generally a really dangerous feature. minimize_lock sounds so friendly,
does
not imply danger really.Alternative naming proposal: unlocked_thus_unsafe
It sounds good idea. How about shorter name?
unsafe_lock
Well, from the user (PHP code) level it is not a lock at all. The localized
flocking is purely for internal consistency of the individual read or write
operation, in that case.
That's why I wanted to have 'unlocked' in the name. And for such a dangerous
option (*) I think that a very expressive long name would be a good fit.
best regards
Patrick
(*) yes I remember that I suggested it in the first place, back in October :)
Hi Patrick,
On Thursday 30 January 2014 14:34:35 Yasuo Ohgaki wrote:
Generally a really dangerous feature. minimize_lock sounds so friendly,
does
not imply danger really.Alternative naming proposal: unlocked_thus_unsafe
It sounds good idea. How about shorter name?
unsafe_lock
Well, from the user (PHP code) level it is not a lock at all. The localized
flocking is purely for internal consistency of the individual read or write
operation, in that case.That's why I wanted to have 'unlocked' in the name. And for such a
dangerous
option (*) I think that a very expressive long name would be a good fit.
I understand the reason. It's descriptive, but 3 words might be too long.
'transaction_lock' might be good enough for users who know DBMS, but it
may not be enough for others.
Does anyone have short and good name?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Does anyone have short and good name?
lock_assure v. lock_advise
lock_stable v. lock_expert
lock_general v. lock_expert
lock_realistic v. lock_optimistic
Just off the top...
-- S.
Does anyone have short and good name?
Why short? Better self explaining than cryptic names, it is not like
one edits his ini settings ten times per day :)
--
Pierre
@pierrejoye | http://www.libgd.org
Am 30.01.2014 06:53 schrieb "Yasuo Ohgaki" yohgaki@ohgaki.net:
Alternative naming proposal: unlocked_thus_unsafe
It sounds good idea. How about shorter name?
unsafe_lock
Well, from the user (PHP code) level it is not a lock at all. The
localized
flocking is purely for internal consistency of the individual read or
write
operation, in that case.That's why I wanted to have 'unlocked' in the name. And for such a
dangerous
option (*) I think that a very expressive long name would be a good fit.I understand the reason. It's descriptive, but 3 words might be too long.
'transaction_lock' might be good enough for users who know DBMS, but it
may not be enough for others.Does anyone have short and good name?
If you insist on it being short/shorter, I'd go for
unlocked_unsafe
or
unlocked
because I think, and keep thinking, that having "unlocked" there best
defuses the traditional expectation that sessions are locked during the
whole script run - thus minimizing the surprise to programmers at the PHP
level.
best regards
Patrick
Hi all,
If you insist on it being short/shorter, I'd go for
unlocked_unsafe
or
unlocked
because I think, and keep thinking, that having "unlocked" there best
defuses the traditional expectation that sessions are locked during the
whole script run - thus minimizing the surprise to programmers at the PHP
level.
Thank you for suggestions!
I would like to use
unlock
BTW, I've the code. It's checking fd, so it serialized :)
Thank you.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
If you insist on it being short/shorter, I'd go for
unlocked_unsafe
or
unlocked
because I think, and keep thinking, that having "unlocked" there best
defuses the traditional expectation that sessions are locked during the
whole script run - thus minimizing the surprise to programmers at the PHP
level.Thank you for suggestions!
I would like to useunlock
How about 'unsafe_lock'?
It may be better.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
This is informational function for programmers, not for programs.
It requires active session to be useful because it needs active save handler
module. It's rather suitable asphpinfo(), but it might not be useful
for users.
I'd say it's better to leave this for the docs. If you write session
handler, you document what it's supporting and what's not and that would
be much easier to read than call a function and also much clearer -
there you can describe what works and what not in detail.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Stas,
On Thu, Jan 30, 2014 at 4:16 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
This is informational function for programmers, not for programs.
It requires active session to be useful because it needs active save
handler
module. It's rather suitable asphpinfo(), but it might not be useful
for users.I'd say it's better to leave this for the docs. If you write session
handler, you document what it's supporting and what's not and that would
be much easier to read than call a function and also much clearer -
there you can describe what works and what not in detail.
No problem. I think it's dirty also :)
Let leave it to docs. It will be removed from code and RFC.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
Revised RFC is prepared and the patch is there. I would like to start vote
again.
Is there any new comment for the RFC?
https://wiki.php.net/rfc/session-lock-ini
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi all,
New session feature has been discussed while.
Vote period: 2014/01/20 - 2014/01/30
https://wiki.php.net/rfc/session-lock-iniNew comments and questions are welcomed.
Thank you for voting.
P.S. I have removed session_discard(). I've committed
it assession_abort()to address FR last year. Name/alias
is open to be changed. Please comment on different thread
for this.--
Yasuo Ohgaki
yohgaki@ohgaki.net