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 1 commit
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 34 additions & 9 deletions ext/session/session.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,24 +485,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

@laruence laruence Apr 8, 2016

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

@yohgaki yohgaki Apr 8, 2016

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

@laruence laruence Apr 8, 2016

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;
int nrdels = -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());
Copy link
Member

@krakjoe krakjoe Apr 7, 2016

Choose a reason for hiding this comment

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

This can be moved after the immediate branch.
Copy link
Contributor Author

@yohgaki yohgaki Apr 7, 2016

Choose a reason for hiding this comment

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

Oops. Thanks!
if (immediate) {
Copy link
Contributor

@narfbg narfbg Apr 7, 2016

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

@smalyshev smalyshev Apr 7, 2016

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

@yohgaki yohgaki Apr 8, 2016

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), &nrdels);
return (zend_long)nrdels;
}
Copy link
Member

@krakjoe krakjoe Apr 7, 2016

Choose a reason for hiding this comment

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

Is there a good reason to remove this ?
Copy link
Contributor Author

@yohgaki yohgaki Apr 7, 2016

Choose a reason for hiding this comment

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

Not really. I just never used it since debugger is much easier rather than adding def and recompile.
Copy link
Member

@krakjoe krakjoe Apr 7, 2016

Choose a reason for hiding this comment

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

hmm, I thought there were others ... but this seems to be the only one ...

I guess it can go, though it doesn't really need to be part of this PR/RFC.
Copy link
Contributor Author

@yohgaki yohgaki Apr 7, 2016

Choose a reason for hiding this comment

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

Yes. Indeed, it's the only one left ;)
I'll just remove and commit it as normal maintenance.
if (nrand < PS(gc_probability)) {
PS(mod)->s_gc(&PS(mod_data), PS(gc_maxlifetime), &nrdels);
#ifdef SESSION_DEBUG
if (nrdels != -1) {
php_error_docref(NULL, E_NOTICE, "purged %d expired session objects", nrdels);
}
#endif
}
}
return (zend_long)nrdels;
} /* }}} */


Expand Down Expand Up @@ -568,7 +567,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 @@ -2381,6 +2380,31 @@ 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 nrdels;
Copy link
Member

@laruence laruence Apr 7, 2016

Choose a reason for hiding this comment

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

what does this means? maybe a simple num is better.
Copy link
Contributor Author

@yohgaki yohgaki Apr 7, 2016

Choose a reason for hiding this comment

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

Fine with me. I'm using nrdels because the name is used for "number of deleted sessions" in the code.
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

@smalyshev smalyshev Apr 7, 2016

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

@yohgaki yohgaki Apr 8, 2016

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

@narfbg narfbg Apr 8, 2016

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

@yohgaki yohgaki Apr 8, 2016

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

@yohgaki yohgaki Apr 8, 2016

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

@narfbg narfbg Apr 8, 2016

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

@yohgaki yohgaki Apr 8, 2016

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

@narfbg narfbg Apr 8, 2016

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

@smalyshev smalyshev Apr 18, 2016

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

@yohgaki yohgaki Aug 25, 2016

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;
}

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

RETURN_LONG(nrdels);
}
/* }}} */


/* {{{ proto void session_write_close(void)
Write session data and end session */
static PHP_FUNCTION(session_write_close)
Expand Down Expand Up @@ -2558,6 +2582,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
43 changes: 43 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,43 @@
--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";

// Turn off strice mode, since it does not allow uninitialized session ID
ini_set('session.use_strict_mode',false);

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 16
bool(false)
bool(true)
int(%d)
bool(true)
bool(true)
string(0) ""
Done