FAQ
Hello internals,

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.

Regards,
Andrey Andreev.

Search Discussions

  • Yasuo Ohgaki at Mar 16, 2014 at 1:12 am
    Hi Andrey,
    On Sun, Mar 16, 2014 at 3:24 AM, Andrey Andreev wrote:

    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
  • Andrey Andreev at Mar 16, 2014 at 1:27 am
    "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.
  • Stas Malyshev at Mar 16, 2014 at 1:28 am
    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:

    1. My request is never going to change the state => use read_only
    2. My request might change the state and thus I need exclusive lock =>
    use lazy_write
    3. 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
  • Yasuo Ohgaki at Mar 16, 2014 at 1:45 am

    On Sun, Mar 16, 2014 at 10:28 AM, Stas Malyshev wrote:

    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
  • Andrey Andreev at Mar 16, 2014 at 4:34 pm
    Hi,
    On Sun, Mar 16, 2014 at 3:28 AM, Stas Malyshev wrote:
    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.
    1. My request is never going to change the state => use read_only
    2. My request might change the state and thus I need exclusive lock =>
    use lazy_write
    3. 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.
  • Stas Malyshev at Mar 16, 2014 at 8:03 pm
    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
  • Andrey Andreev at Mar 16, 2014 at 11:07 pm
    Hi,
    On Sun, Mar 16, 2014 at 10:03 PM, Stas Malyshev wrote:
    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.
  • Yasuo Ohgaki at Mar 17, 2014 at 12:10 am
    Hi Andrey,
    On Mon, Mar 17, 2014 at 8:07 AM, Andrey Andreev wrote:

    "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
  • Andrey Andreev at Mar 17, 2014 at 8:52 am
    Hi,
    On Mon, Mar 17, 2014 at 2:09 AM, Yasuo Ohgaki wrote:
    Hi Andrey,
    On Mon, Mar 17, 2014 at 8:07 AM, Andrey Andreev wrote:

    "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?
  • Yasuo Ohgaki at Mar 17, 2014 at 9:02 am
    Hi Andrey,
    On Mon, Mar 17, 2014 at 5:52 PM, Andrey Andreev wrote:

    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
  • Andrey Andreev at Mar 17, 2014 at 9:14 am
    Hi,
    On Mon, Mar 17, 2014 at 11:02 AM, Yasuo Ohgaki wrote:
    Hi Andrey,
    On Mon, Mar 17, 2014 at 5:52 PM, Andrey Andreev wrote:

    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.
  • Yasuo Ohgaki at Mar 17, 2014 at 9:32 am
    Hi Andrey,
    On Mon, Mar 17, 2014 at 6:14 PM, Andrey Andreev wrote:

    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
  • Andrey Andreev at Mar 17, 2014 at 9:58 am
    Hi,
    On Mon, Mar 17, 2014 at 11:31 AM, Yasuo Ohgaki wrote:
    Hi Andrey,
    On Mon, Mar 17, 2014 at 6:14 PM, Andrey Andreev wrote:

    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.
  • Yasuo Ohgaki at Mar 17, 2014 at 10:36 am
    Hi all,
    On Mon, Mar 17, 2014 at 6:57 PM, Andrey Andreev wrote:
    On Mon, Mar 17, 2014 at 11:31 AM, Yasuo Ohgaki wrote:
    On Mon, Mar 17, 2014 at 6:14 PM, Andrey Andreev 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
  • Andrey Andreev at Mar 17, 2014 at 10:47 am
    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.
  • Yasuo Ohgaki at Mar 18, 2014 at 12:51 am
    Hi all,
    On Mon, Mar 17, 2014 at 7:47 PM, Andrey Andreev wrote:

    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
  • Stas Malyshev at Mar 18, 2014 at 5:45 am
    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
  • Yasuo Ohgaki at Mar 18, 2014 at 7:44 am
    Hi Stas,

    Thank you for the comment on github.
    On Tue, Mar 18, 2014 at 2:45 PM, Stas Malyshev wrote:

    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
  • Andrey Andreev at Mar 18, 2014 at 10:58 am
    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.
  • Ferenc Kovacs at Mar 18, 2014 at 6:45 pm
    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.
  • Yasuo Ohgaki at Mar 18, 2014 at 11:05 pm
    Hi all,
    On Tue, Mar 18, 2014 at 7:58 PM, Andrey Andreev 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.

    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
  • Andrey Andreev at Mar 18, 2014 at 11:27 pm

    On Wed, Mar 19, 2014 at 1:04 AM, Yasuo Ohgaki wrote:
    Hi all,
    On Tue, Mar 18, 2014 at 7:58 PM, Andrey Andreev 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.
    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.
  • Yasuo Ohgaki at Mar 18, 2014 at 11:51 pm
    Hi Andrey,
    On Wed, Mar 19, 2014 at 8:27 AM, Andrey Andreev wrote:
    On Wed, Mar 19, 2014 at 1:04 AM, Yasuo Ohgaki wrote:
    Hi all,
    On Tue, Mar 18, 2014 at 7:58 PM, Andrey Andreev 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
  • Andrey Andreev at Mar 19, 2014 at 12:17 am
    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.
  • Yasuo Ohgaki at Mar 19, 2014 at 1:39 am
    Hi Andrey,
    On Wed, Mar 19, 2014 at 9:17 AM, Andrey Andreev wrote:

    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
  • Andrey Andreev at Mar 20, 2014 at 11:19 am
    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:

    1. 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).

    2. 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.
  • John LeSueur at Mar 20, 2014 at 6:35 pm

    On Thu, Mar 20, 2014 at 5:19 AM, Andrey Andreev wrote:

    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:

    1. 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).

    2. 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.

    --
    PHP Internals - PHP Runtime Development Mailing List
    To unsubscribe, visit: http://www.php.net/unsub.php
    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
  • Andrey Andreev at Mar 21, 2014 at 1:46 pm
    Hi,
    On Thu, Mar 20, 2014 at 8:35 PM, John LeSueur wrote:
    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.
  • Yasuo Ohgaki at Mar 16, 2014 at 1:40 am
    Hi Andrey,
    On Sun, Mar 16, 2014 at 10:12 AM, Yasuo Ohgaki wrote:

    Hi Andrey,
    On Sun, Mar 16, 2014 at 3:24 AM, Andrey Andreev wrote:

    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
  • Yasuo Ohgaki at Mar 16, 2014 at 5:33 am
    Hi Andrey,

    I've finished modifying patch, so I read your RFC.
    On Sun, Mar 16, 2014 at 3:24 AM, Andrey Andreev wrote:

    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.)


    2. 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
  • Andrey Andreev at Mar 16, 2014 at 4:48 pm
    Hi,
    On Sun, Mar 16, 2014 at 7:32 AM, Yasuo Ohgaki wrote:
    // $_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.
    2. 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.
  • Yasuo Ohgaki at Mar 16, 2014 at 7:57 am

    On Sun, Mar 16, 2014 at 3:24 AM, Andrey Andreev wrote:

    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.
    1. 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
  • Stas Malyshev at Mar 16, 2014 at 7:59 pm
    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

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMar 15, '14 at 6:24p
activeMar 21, '14 at 1:46p
posts34
users5
websitephp.net

People

Translate

site design / logo © 2022 Grokbase