Skip to content

Add session_gc() #1852

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from
Closed

Add session_gc() #1852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6737717
add session_gc()
Apr 7, 2016
abbc9cf
Adjust code and test
Apr 7, 2016
dc130ee
Merge remote-tracking branch 'upstream/master' into master-rfc-sessio…
Apr 7, 2016
bf49371
Fix php_session_gc()
yohgaki Apr 8, 2016
c398b0d
Last commit had wrongly removed gc_maxlifetime. Original code was cor…
yohgaki Apr 8, 2016
fbe15ee
Merge remote-tracking branch 'upstream/master' into master-rfc-sessio…
Aug 25, 2016
e22cf71
Use zend_long simply. Made gc handler can return number of deleted re…
Aug 25, 2016
15e143b
Merge branch 'master-rfc-session-gc' of github.com:yohgaki/php-src in…
Aug 25, 2016
5455c9f
Adjust code a little
Aug 25, 2016
9614406
Hanble user handler return value for gc func more compatible manner
Aug 25, 2016
c3b19c5
Fix comment
Aug 25, 2016
3e8792f
Return FALSE to be more compatible
Aug 25, 2016
1015d9c
Negative number of deleted sessions is the error condition.
Aug 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ext/session/mod_files.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,9 +643,11 @@ PS_GC_FUNC(files)

if (data->dirdepth == 0) {
*nrdels = ps_files_cleanup_dir(data->basedir, maxlifetime);
} else {
*nrdels = -1; // Cannot process multiple depth save dir
}

return SUCCESS;
return *nrdels;
}


Expand Down
2 changes: 1 addition & 1 deletion ext/session/mod_mm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,7 @@ PS_GC_FUNC(mm)

mm_unlock(data->mm);

return SUCCESS;
return nrdels;
}

PS_CREATE_SID_FUNC(mm)
Expand Down
13 changes: 11 additions & 2 deletions ext/session/mod_user.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,22 @@ PS_DESTROY_FUNC(user)
PS_GC_FUNC(user)
{
zval args[1];
STDVARS;
zval retval;

ZVAL_LONG(&args[0], maxlifetime);

ps_call_handler(&PSF(gc), 1, args, &retval);

FINISH;
if (Z_TYPE(retval) == IS_LONG) {
convert_to_long(&retval);
return Z_LVAL(retval);
}
/* This is for older API compatibility */
if (Z_TYPE(retval) == IS_TRUE) {
return 1;
}
/* Anything else is some kind of error */
return -1; // Error
}

PS_CREATE_SID_FUNC(user)
Expand Down
7 changes: 5 additions & 2 deletions ext/session/mod_user_class.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,15 +148,18 @@ PHP_METHOD(SessionHandler, destroy)
PHP_METHOD(SessionHandler, gc)
{
zend_long maxlifetime;
int nrdels;
zend_long nrdels = -1;

PS_SANITY_CHECK_IS_OPEN;

if (zend_parse_parameters(ZEND_NUM_ARGS(), "l", &maxlifetime) == FAILURE) {
return;
}

RETURN_BOOL(SUCCESS == PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels));
if (PS(default_mod)->s_gc(&PS(mod_data), maxlifetime, &nrdels) == FAILURE) {
RETURN_FALSE;
}
RETURN_LONG(nrdels);
}
/* }}} */

Expand Down
6 changes: 3 additions & 3 deletions ext/session/php_session.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
#define PS_READ_ARGS void **mod_data, zend_string *key, zend_string **val, zend_long maxlifetime
#define PS_WRITE_ARGS void **mod_data, zend_string *key, zend_string *val, zend_long maxlifetime
#define PS_DESTROY_ARGS void **mod_data, zend_string *key
#define PS_GC_ARGS void **mod_data, zend_long maxlifetime, int *nrdels
#define PS_GC_ARGS void **mod_data, zend_long maxlifetime, zend_long *nrdels
#define PS_CREATE_SID_ARGS void **mod_data
#define PS_VALIDATE_SID_ARGS void **mod_data, zend_string *key
#define PS_UPDATE_TIMESTAMP_ARGS void **mod_data, zend_string *key, zend_string *val, zend_long maxlifetime
Expand All @@ -51,7 +51,7 @@ typedef struct ps_module_struct {
int (*s_read)(PS_READ_ARGS);
int (*s_write)(PS_WRITE_ARGS);
int (*s_destroy)(PS_DESTROY_ARGS);
int (*s_gc)(PS_GC_ARGS);
zend_long (*s_gc)(PS_GC_ARGS);
zend_string *(*s_create_sid)(PS_CREATE_SID_ARGS);
int (*s_validate_sid)(PS_VALIDATE_SID_ARGS);
int (*s_update_timestamp)(PS_UPDATE_TIMESTAMP_ARGS);
Expand All @@ -65,7 +65,7 @@ typedef struct ps_module_struct {
#define PS_READ_FUNC(x) int ps_read_##x(PS_READ_ARGS)
#define PS_WRITE_FUNC(x) int ps_write_##x(PS_WRITE_ARGS)
#define PS_DESTROY_FUNC(x) int ps_delete_##x(PS_DESTROY_ARGS)
#define PS_GC_FUNC(x) int ps_gc_##x(PS_GC_ARGS)
#define PS_GC_FUNC(x) zend_long ps_gc_##x(PS_GC_ARGS)
#define PS_CREATE_SID_FUNC(x) zend_string *ps_create_sid_##x(PS_CREATE_SID_ARGS)
#define PS_VALIDATE_SID_FUNC(x) int ps_validate_sid_##x(PS_VALIDATE_SID_ARGS)
#define PS_UPDATE_TIMESTAMP_FUNC(x) int ps_update_timestamp_##x(PS_UPDATE_TIMESTAMP_ARGS)
Expand Down
47 changes: 39 additions & 8 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,19 +353,23 @@ PHPAPI int php_session_valid_key(const char *key) /* {{{ */
/* }}} */


static void php_session_gc(void) /* {{{ */
static zend_long php_session_gc(zend_bool immediate) /* {{{ */
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why zend_long here, and lots of casting below...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP 7 and up uses zend_long and I would like to get zend_long return value for "num of deleted session" from 3rd party handler in the future.

It could be int for now. If you insist, I don't mind at all using int here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in that case, why you don't use zend_long num? I don't care about zend_long or int, just seems a little ugly you are doing casting without a good reason.

Copy link
Contributor Author

@yohgaki yohgaki Aug 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree casting is ugly.
I'll simply use zend_long. I'll commit changes soon. Please review.

BTW, sorry for the delay. I didn't see notification mails...

Copy link
Contributor Author

@yohgaki yohgaki Aug 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated patch so that it can simply use zend_long.

User handlers are modified to be able to return LONG or BOOL. User GC functions are supposed to be implemented just like 3rd party C save handlers by returning LONG (0 > for number of deleted session, negative number of errors). FALSE is treated as negative num of deleted sessions. TRUE is treated as 1. Since return value of user handler should not be used by user code, there is no BC.

NOTE: Negative number is used to indicate GC errors for a long time. (IIRC, it's since session module was introduced)

int nrand;
zend_long num = -1;

/* GC must be done before reading session data. */
if ((PS(mod_data) || PS(mod_user_implemented)) && PS(gc_probability) > 0) {
int nrdels = -1;

nrand = (int) ((float) PS(gc_divisor) * php_combined_lcg());
if (nrand < PS(gc_probability)) {
PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &nrdels);
if ((PS(mod_data) || PS(mod_user_implemented))) {
if (immediate) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this beat the point of checking gc_probability? If so, shouldn't it be outside of the previous if() condition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, this looks wrong. If you want immediate cleanup, no reason to check probability. Better would be to split the check and cleanup into separate functions and call the latter directly when you need unconditional cleanup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right it does not needed at all. I'll fix it later :)

PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num);
return num;
}
nrand = (zend_long) ((float) PS(gc_divisor) * php_combined_lcg());
if (PS(gc_probability) > 0 && nrand < PS(gc_probability)) {
PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &num);
}
}
return num;
} /* }}} */

static void php_session_initialize(void) /* {{{ */
Expand Down Expand Up @@ -430,7 +434,7 @@ static void php_session_initialize(void) /* {{{ */
}

/* GC must be done after read */
php_session_gc();
php_session_gc(0);

if (PS(session_vars)) {
zend_string_release(PS(session_vars));
Expand Down Expand Up @@ -2239,6 +2243,32 @@ static PHP_FUNCTION(session_unset)
}
/* }}} */

/* {{{ proto int session_gc(void)
Perform GC and return number of deleted sessions */
static PHP_FUNCTION(session_gc)
{
zend_long num;

if (zend_parse_parameters_none() == FAILURE) {
return;
}

if (PS(session_status) != php_session_active) {
php_error_docref(NULL, E_WARNING, "Session is not active");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why you need session to be active for GC to happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because save handler must be activated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? And can't that be changed?
IMO it's an ugly hack if I have to call session_start() from inside a cronjob.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

session_gc() handler is defined in save handlers. Unless save handler storage is initialized, gc function won't work.
If you don't like stray sessions by cron tasks, you can use static session ID for the task.

e.g.

However, since this is C code, I can write code that open, gc, close session. In this case, session must be inactive. Otherwise, I'll destroy save handler data structure.

If above code is written. GC cron tack code would be

Modules should do more work rather than users. I'll implement it. Is this satisfactory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arfbg Andrey, if above code is written

session_start();
session_gc(); // won't work

There may be users write

session_gc();
session_start();

though ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can still check if the session is already active and don't call close() if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may work with many save handlers, but I cannot control how 3rd party/user save handlers are implemented. It's risky. Taking risk does not worth, IMO. I've probably seen too many unexpected usages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pseudo-code:

if (session_status() === PHP_SESSION_ACTIVE)
{
    $sessionHandler->gc();
}
else
{
    $sessionHandler->open();
    $sessionHandler->gc();
    $sessionHandler->close();
}

How can that be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Static session ID" hack looks very ugly. If we need init happen before we run GC, we should hide it from the user, instead of requiring them to write boilerplate code that makes absolutely no sense for them ("why I need to start dummy session to just run GC?!")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll document "session_gc() is supposed to be called from periodical task manager such as cron".

IMHO, users shouldn't care extra new session ID created by session_gc() because many useless session IDs are created by cookie disabled browsers anyway, for example.

RETURN_FALSE;
}

num = php_session_gc(1);
if (num < 0) {
php_error_docref(NULL, E_WARNING, "Failed to perfom session GC");
RETURN_FALSE;
}

RETURN_LONG(num);
}
/* }}} */


/* {{{ proto void session_write_close(void)
Write session data and end session */
static PHP_FUNCTION(session_write_close)
Expand Down Expand Up @@ -2416,6 +2446,7 @@ static const zend_function_entry session_functions[] = {
PHP_FE(session_start, arginfo_session_void)
PHP_FE(session_destroy, arginfo_session_void)
PHP_FE(session_unset, arginfo_session_void)
PHP_FE(session_gc, arginfo_session_void)
PHP_FE(session_set_save_handler, arginfo_session_set_save_handler)
PHP_FE(session_cache_limiter, arginfo_session_cache_limiter)
PHP_FE(session_cache_expire, arginfo_session_cache_expire)
Expand Down
40 changes: 40 additions & 0 deletions ext/session/tests/session_gc_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
--TEST--
Test session_gc() function : basic functionality
--SKIPIF--
<?php include('skipif.inc'); ?>
--FILE--
<?php

ob_start();

/*
* Prototype : int session_gc(void)
* Description : Perform GC
* Source code : ext/session/session.c
*/

echo "*** Testing session_gc() : basic functionality ***\n";

var_dump(session_gc());

var_dump(session_start());
var_dump(session_gc(), session_gc() >= -1);
var_dump(session_destroy());
var_dump(session_id());

echo "Done";
ob_end_flush();
?>
--EXPECTF--
*** Testing session_gc() : basic functionality ***

Warning: session_gc(): Session is not active in %s on line %d
bool(false)
bool(true)
int(%d)
bool(true)
bool(true)
string(0) ""
Done