Hi,
I was just reviewing the new code and functions in 5.6 for session module.
I noticed that session_reset() and session_abort() came in.
I would suggest those functions to throw errors when the session is not
initialized.
This is already done for all other session related functions
(session_reset_id(), session_regenerate_id(), session_destroy()).
Thoughts ?
Julien Pauli
Hi Julien,
I was just reviewing the new code and functions in 5.6 for session module.
I noticed that
session_reset()andsession_abort()came in.I would suggest those functions to throw errors when the session is not
initialized.This is already done for all other session related functions
(session_reset_id(),session_regenerate_id(),session_destroy()).Thoughts ?
I agree and they should.
I'll take care of them or anyone else?
It will be few days later.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Julien,
I was just reviewing the new code and functions in 5.6 for session module.
I noticed that
session_reset()andsession_abort()came in.I would suggest those functions to throw errors when the session is not
initialized.This is already done for all other session related functions
(session_reset_id(),session_regenerate_id(),session_destroy()).Thoughts ?
I agree and they should.
I'll take care of them or anyone else?
It will be few days later.
You can go for it, I have no git connection today :-p
Thank you !
Julien
Hi,
I had previously requested the removal of these 2 functions based on
lack of discussion about them (or at least I found none). My request
was ignored because I had to tell "why it's not needed", which I don't
think is the right approach for a new feature that somebody just
decided to commit, but regardless ... here's a good reason:
From what I understand, session_reset() not only discards changes to
$_SESSION and not just re-reads the data, but re-initializes the whole
session. This includes calls to open(), read(), which include opening
file pointers, setting locks, checking session ID validity, etc. I
don't know how safe that is to do internally, but with a userland
session handler, it means discarding previously open connections/file
pointers, locks and whoever knows what else a developer might've
cached, assuming that open(), read() would only be called once.
It would've also been better IMO if the function was called
session_restart() instead, because of the above-described behavior.
I also don't like that these 2 functions kind of make sessions to look
like a traditional database transaction, or at least I'd hate it to
see code that uses them for the wrong purpose, but that's rather
philosophical ... I do see them as useful for resetting the
application to a certain state in case of an error, but in that
context - session_abort() alone should be sufficient.
Cheers,
Andrey.
Hi,
I had previously requested the removal of these 2 functions based on
lack of discussion about them (or at least I found none). My request
was ignored because I had to tell "why it's not needed", which I don't
think is the right approach for a new feature that somebody just
decided to commit, but regardless ... here's a good reason:From what I understand,
session_reset()not only discards changes to
$_SESSION and not just re-reads the data, but re-initializes the whole
session. This includes calls to open(), read(), which include opening
file pointers, setting locks, checking session ID validity, etc. I
don't know how safe that is to do internally, but with a userland
session handler, it means discarding previously open connections/file
pointers, locks and whoever knows what else a developer might've
cached, assuming that open(), read() would only be called once.
It would've also been better IMO if the function was called
session_restart() instead, because of the above-described behavior.I also don't like that these 2 functions kind of make sessions to look
like a traditional database transaction, or at least I'd hate it to
see code that uses them for the wrong purpose, but that's rather
philosophical ... I do see them as useful for resetting the
application to a certain state in case of an error, but in that
context -session_abort()alone should be sufficient.
Yup, I agree that I dont really see a use case for session_reset().
The implementation looks strange.
Yasuo, could you clarify please ?
Julien
Hi all,
Yup, I agree that I dont really see a use case for
session_reset().
The implementation looks strange.Yasuo, could you clarify please ?
This function is useful for session save handlers that do not lock
session data. Example is memcached. This function could be used
re-read session data to mitigate over written session data with unlock
session data.
Andrey, just because you don't think of usage, it does not mean it does not
mean unneeded or not useful. You are better to ask the reason behind why
first.
Good library should have defined API for specific tasks, too. You also has
misunderstanding about why delayed deletion for session_regenerate_id()
is mandatory.
BTW, raising errors for invalid calls has issues, so I send PR to discuss.
https://github.com/php/php-src/pull/634
Regards,
P.S. If I should choose to remove this or session_gc(), I choose this
because
session_gc() is mandatory API for decent session manager.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
This function is useful for session save handlers that do not lock
session data. Example is memcached. This function could be used
re-read session data to mitigate over written session data with unlock
session data.Andrey, just because you don't think of usage, it does not mean it does not
mean unneeded or not useful. You are better to ask the reason behind why
first.
Good library should have defined API for specific tasks, too. You also has
misunderstanding about why delayed deletion forsession_regenerate_id()
is mandatory.
Yasuo ... There's no misunderstanding, I just have a disagreement with
you about that issue and I'm tired of arguing over that. Let's just
keep session_regenerate_id() in it's own thread, ok?
But speaking of mandatory: locks are mandatory, so how are non-locking
handlers an argument in all of this?
Also, I didn't just question if there's a use case, I'm questioning if
it is safe to use session_reset() at all. Re-reading doesn't imply
discarding currently open resources and complerely re-initializing the
session and from what I see - that's exactly what the function does,
or am I missing something?
Cheers,
Andrey.
Hi Andrey,
This function is useful for session save handlers that do not lock
session data. Example is memcached. This function could be used
re-read session data to mitigate over written session data with unlock
session data.Andrey, just because you don't think of usage, it does not mean it does
not
mean unneeded or not useful. You are better to ask the reason behind why
first.
Good library should have defined API for specific tasks, too. You also
has
misunderstanding about why delayed deletion forsession_regenerate_id()
is mandatory.Yasuo ... There's no misunderstanding, I just have a disagreement with
you about that issue and I'm tired of arguing over that. Let's just
keepsession_regenerate_id()in it's own thread, ok?
Ok.
I'm talking about XHR cannot be solution and delayed deletion is
mandatory for precise session management. We may discuss later since
I'm going to fork session module.
But speaking of mandatory: locks are mandatory, so how are non-locking
handlers an argument in all of this?
I use unlocked session for performance tuning often.
Apps could be much faster with little care and little code changes.
Sometimes this function is needed to be sure.
Also, I didn't just question if there's a use case, I'm questioning if
it is safe to use
session_reset()at all. Re-reading doesn't imply
discarding currently open resources and complerely re-initializing the
session and from what I see - that's exactly what the function does,
or am I missing something?
I think session manager should "manage" how session is managed.
Save handlers should save/retrieve session data only. Since session
manager does not have such feature, save handlers does the task and
there is unlocked sessions. Since there is no exposed user functions for
save handlers, reset is needed on occasion.
I would rather have session_gc() back as I wrote in previous mail, though.
This one is truly mandatory function even with precise session data
expiration.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Andrey,
From what I understand,
session_reset()not only discards changes to
$_SESSION and not just re-reads the data, but re-initializes the whole
session. This includes calls to open(), read(), which include opening
file pointers, setting locks, checking session ID validity, etc. I
don't know how safe that is to do internally, but with a userland
session handler, it means discarding previously open connections/file
pointers, locks and whoever knows what else a developer might've
cached, assuming that open(), read() would only be called once.
It would've also been better IMO if the function was called
session_restart() instead, because of the above-described behavior.
It's safe. Don't worry.
As always, I prefer better names.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Andrey,
From what I understand,
session_reset()not only discards changes to
$_SESSION and not just re-reads the data, but re-initializes the whole
session. This includes calls to open(), read(), which include opening
file pointers, setting locks, checking session ID validity, etc. I
don't know how safe that is to do internally, but with a userland
session handler, it means discarding previously open connections/file
pointers, locks and whoever knows what else a developer might've
cached, assuming that open(), read() would only be called once.
It would've also been better IMO if the function was called
session_restart() instead, because of the above-described behavior.It's safe. Don't worry.
As always, I prefer better names.
Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort() && session_start() ?
Cheers,
Andrey.
Hi Andrey,
Hi Andrey,
On Wed, Mar 26, 2014 at 1:06 AM, Andrey Andreev narf@devilix.net
wrote:From what I understand,
session_reset()not only discards changes to
$_SESSION and not just re-reads the data, but re-initializes the whole
session. This includes calls to open(), read(), which include opening
file pointers, setting locks, checking session ID validity, etc. I
don't know how safe that is to do internally, but with a userland
session handler, it means discarding previously open connections/file
pointers, locks and whoever knows what else a developer might've
cached, assuming that open(), read() would only be called once.
It would've also been better IMO if the function was called
session_restart() instead, because of the above-described behavior.It's safe. Don't worry.
As always, I prefer better names.Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort()&&session_start()?
Yes, it's an API for it.
It's more efficient than user land functions as it requires needless
close and open (and initialization required to open).
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort()&&session_start()?Yes, it's an API for it.
It's more efficient than user land functions as it requires needless
close and open (and initialization required to open).
I'm confused now, does it call open() or does it not?
If it doesn't - it's unsafe.
If it does - it's unclear, redundant and optimizes by silently
discarding already open resources (without properly closing them).
Either way, I like efficiency but I also see it as flawed.
Cheers,
Andrey.
Hi Andrey,
Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort()&&session_start()?Yes, it's an API for it.
It's more efficient than user land functions as it requires needless
close and open (and initialization required to open).I'm confused now, does it call open() or does it not?
If it doesn't - it's unsafe.
It's safe as long as handler locks data during read/write.
Example is memcached save handler with unlocked session.
If it does - it's unclear, redundant and optimizes by silently
discarding already open resources (without properly closing them).
Either way, I like efficiency but I also see it as flawed.
Save handler may lock/unlock session data. There is no standardized
way for it.
Since there would not be lock option for session manager, I don't mind
removing it. There is not much point to have it without lock option for
session manager. I would like to have session_gc() than session_reset().
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Andrey,
Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort()&&session_start()?Yes, it's an API for it.
It's more efficient than user land functions as it requires needless
close and open (and initialization required to open).I'm confused now, does it call open() or does it not?
If it doesn't - it's unsafe.It's safe as long as handler locks data during read/write.
Example is memcached save handler with unlocked session.
I had something other in mind and you didn't really answer my
question, but ... there you have it - it can be unsafe even by your
own words. :)
If it does - it's unclear, redundant and optimizes by silently
discarding already open resources (without properly closing them).Either way, I like efficiency but I also see it as flawed.
Save handler may lock/unlock session data. There is no standardized
way for it.Since there would not be lock option for session manager, I don't mind
removing it. There is not much point to have it without lock option for
session manager. I would like to havesession_gc()thansession_reset().
This sounds a bit like you're proposing some kind of a trade
agreement. When you put it that way, I'd also rather have session_gc()
instead of session_reset(), but that's not the point. My initial
argument was that these features need to be brainstormed here on
internals and there was no discussion about them at all.
Cheers,
Andrey.
Hi Andrey,
Hi Andrey,
Well, I just re-checked it and does indeed just call
php_session_initialize(), so what's the point? Shorthand for
session_abort()&&session_start()?Yes, it's an API for it.
It's more efficient than user land functions as it requires needless
close and open (and initialization required to open).I'm confused now, does it call open() or does it not?
If it doesn't - it's unsafe.It's safe as long as handler locks data during read/write.
Example is memcached save handler with unlocked session.I had something other in mind and you didn't really answer my
question, but ... there you have it - it can be unsafe even by your
own words. :)
Regarding to safety of session data management, session management is not
safe anyway. Locked session does not guarantee data consistency due to
nature of HTTP session management. Locked session provides a litter better
consistency. That's all and no more than that. i.e. It's false sense of
safety.
That said better is better. That's the reason why I'm proposing various
better
way of management than now.
If it does - it's unclear, redundant and optimizes by silently
discarding already open resources (without properly closing them).
Either way, I like efficiency but I also see it as flawed.
Save handler may lock/unlock session data. There is no standardized
way for it.Since there would not be lock option for session manager, I don't mind
removing it. There is not much point to have it without lock option for
session manager. I would like to havesession_gc()thansession_reset().This sounds a bit like you're proposing some kind of a trade
agreement. When you put it that way, I'd also rather havesession_gc()
instead ofsession_reset(), but that's not the point. My initial
argument was that these features need to be brainstormed here on
internals and there was no discussion about them at all.
It's not trade. session_gc() is mandatory and I'll add it anyway :)
Periodic GC is much better way than probability based GC. There must
be API for it. I think there wouldn't be argument for this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net