Hi Stas,
On Tue, Apr 19, 2016 at 3:55 AM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>>> Document calling session_gc() periodically is the best practice.
>>
>> If you want to document usage of this new API as the best practice, it
>> would be unfair to the users if you don't also document the caveats that
>> come with it:
>
> I also think it's wrong to document it so. While having periodical GC is
> the best practice, probabilistic GC *is* calling GC periodically. True,
> for some exceptional scenarios - like sites with very low traffic - the
> period is unpredictable. And *then* it's recommended to explicitly call
> session_gc(), and for these cases it is a great addition. But in a
> typical medium or high traffic site the period is very predictable as as
> such there's no way to document otherwise.
Problem of session GC is not only "when GC is performed" but also
"GC affects user experience" i.e. Occasional slow response.
Therefore, GC is better to be performed by non user process/thread. IMHO.
Exceptions are save handlers do not require GC at all. e.g. memcached
>
>> probability-based behavior (and how to do that; based on my
>> observations, the vast majority of users don't know how GC is triggered
>> at all)
>
> And that's great BTW since majority of users don't need to know that -
> it just works. We shouldn't ruin that.
Since we don't have precise session management (yet), users should
aware problems in probability based session GC triggered by user
requests.
Even if we have precise session management, large number of session
data will affect user experience with default session storage. i.e. files
save handler.
BTW, we already have feature that users must remove obsolete session
data files. If user uses multi level dir structure for files save handler,
files save handler will not remove obsolete session data at all.
PS_GC_FUNC(files)
{
PS_FILES_DATA;
/* we don't perform any cleanup, if dirdepth is larger than 0.
we return SUCCESS, since all cleanup should be handled by
an external entity (i.e. find -ctime x | xargs rm) */
if (data->dirdepth == 0) {
*nrdels = ps_files_cleanup_dir(data->basedir, maxlifetime TSRMLS_CC);
}
return SUCCESS;
}
>> - Trigger warnings when session_gc() is called while gc_probability is
>> not 0.
>
> This does not sound like a good idea to me. Why we should make it harder
> for people to use it together? session_gc() has nothing to do with
> gc_probability.
session_gc() will have this signature
int session_gc(void) // returns number of removed sessions
>
>> - To avoid re-activating expired sessions, trigger warnings when
>> session_gc() is called after session_start()
>
> Last time I've looked at the patch, session_gc() was required to be
> called after session start. See:
>
> https://github.com/php/php-src/pull/1852/files#diff-2e4264d70a35458a2241e27f76f4a93cR20
>
> I think this is wrong (see my comments there) and in fact we should not
> expose this complexity to the user - gc() should just do the GC and hide
> the technical details.
I haven't update the patch according to discussion.
I'll update the patch soon.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
