Fix GC issue when session lifetime is set to 0 #4744

lf-uraku-yuki · 2021-05-26T08:18:03Z

Description
Fixes #4169

If the session validity period is larger than 1, it operates as usual, and if it is 0, the setting value on the PHP side is used for GC timing.

Checklist:

MGatner · 2021-05-26T11:57:35Z

I don't really know what this does, but the code does solve the bug. Any other maintainers familiar? Or @lf-uraku-yuki can you offer more explanation/documentation on what is happening here?

MGatner · 2021-05-26T11:58:25Z

Is there any scenario where someone would be setting this to 0 intentionally?

lf-uraku-yuki · 2021-05-26T12:11:01Z

The session expiration time is set to the same value in session.gc_maxlifetime in the current implementation. Therefore, session.gc_maxlifetime will be set to 0 even if you want to set the session expiration date to 0 (= While the browser is open). This side effect occurs with the gc method in each SessionHandler and destroys the existing session.

lf-uraku-yuki · 2021-05-26T12:17:18Z

Note that sessionHandler gc is called every time a session is started and is executed with the probability calculated from session.gc_divisor and session.gc_probability.

https://www.php.net/manual/en/sessionhandler.gc.php

MGatner

Okay I think I understand now. And to answer my own question: no, you would never intentionally set this to 0 while using the Session class. I'll leave the PR in case anyone else wants a look, but I'm good with it.

michalsn

As far as I remember setting 0 will work just fine on Debian and will stop PHP gc entirely. This PR can cause problems for some people since their apps will start using the default value which is 1440.

We probably should mention this at least in the changelog.

MGatner · 2021-05-26T13:39:13Z

Really hard to find info on this, but StackOverflow does seem to agree:

A temporary fix might be to do something like change your session.gc_probability to 0 so the session garbage collecting NEVER happens.

But watch out, on most xampp/ampp/...-setups and some linux destributions it's 0, which means the file will never get deleted until you do it within your script (or dirty via shell)

I think we need some more research before going with this.

nmolinos · 2021-10-03T01:39:35Z

I'm not too keen on this fix as it leaves all stale sessions in the table forever (if $sessionExpiration is set to 0). What I did as a quick fix in my environment was to modify the garbage collection method in the DatabaseHandler class to clean out sessions with timestamps older than 2 days.

public function gc($maxlifetime): bool
{
	//deletes all sessions if $sessionExpiration is set to 0 (on browser close)
	//https://github.com/codeigniter4/CodeIgniter4/issues/4169
	//return ($this->db->table($this->table)->delete('timestamp < ' . (time() - $maxlifetime))) ? true : $this->fail();

	//Instead, clean out stale sessions older than 2 days (ending sessions on browser close or 2 days whichever comes first)
	return ($this->db->table($this->table)->delete('timestamp < UNIX_TIMESTAMP(DATE_SUB(NOW(), INTERVAL 2 day))')) ? true : $this->fail();
}

I think the ideal solution involves having two separate values for these settings. One that holds the client-side session expiration, and another for the server-side session expiration used during garbage collection (in place of my hard-coded 2 days in the above example). It would certainly be difficult to clearly explain to someone in the documentation or configuration file though.

Thoughts?

kenjis · 2021-10-11T06:34:28Z

As far as I remember setting 0 will work just fine on Debian and will stop PHP gc entirely.

If I set $sessionExpiration = 0 and ini_set('session.gc_divisor', (string) 1),
all session files in WRITEPATH . 'session' deleted.

kenjis

I think this PR is okay.
But now there is a conflict. So I can't merge.

@lf-uraku-yuki Can you solve it?

kenjis · 2021-10-11T07:02:04Z

@nmolinos
Thank you for your comment.

it leaves all stale sessions in the table forever (if $sessionExpiration is set to 0).

Why it leaves all stale sessions? if $sessionExpiration is set to 0,
session.gc_maxlifetime will be 1440 (PHP default).
And they will be deleted with probability of 1/100 (PHP default).
Am I something wrong?

kenjis · 2021-10-11T07:05:15Z

@lf-uraku-yuki
You did not GPG signed in your commit.
Can you sign?

See https://github.com/codeigniter4/CodeIgniter4/blob/21e3bca85e1b4ba9fbf633da8f2b50a64fc4bccf/contributing/signing.md

nmolinos · 2021-10-11T07:29:36Z

@nmolinos Thank you for your comment.

it leaves all stale sessions in the table forever (if $sessionExpiration is set to 0).

Why it leaves all stale sessions? if $sessionExpiration is set to 0, session.gc_maxlifetime will be 1440 (PHP default). And they will be deleted with probability of 1/100 (PHP default). Am I something wrong?

Hi @kenjis,

session.gc_maxlifetime would only be set to the PHP default in Session.php:307 if the configuration value wasn't set at all:

if (! isset($this->sessionExpiration)) {
            $this->sessionExpiration = (int) ini_get('session.gc_maxlifetime');
        } else {
            ini_set('session.gc_maxlifetime', (string) $this->sessionExpiration);
        }

So leaving it at zero effectively sets gc_maxlifetime to zero as well.

kenjis · 2021-10-11T08:22:12Z

@nmolinos
I mean if this PR gets merged,
when $sessionExpiration is set to 0, session.gc_maxlifetime would be 1440 (PHP default).
And some stale sessions would be deleted.

		elseif ($this->sessionExpiration > 0)
		{
			ini_set('session.gc_maxlifetime', (string) $this->sessionExpiration);
		}

nmolinos · 2021-10-11T08:32:04Z

@nmolinos I mean if this PR gets merged, when $sessionExpiration is set to 0, session.gc_maxlifetime would be 1440 (PHP default). And some stale sessions would be deleted.
		elseif ($this->sessionExpiration > 0)
		{
			ini_set('session.gc_maxlifetime', (string) $this->sessionExpiration);
		}

@kenjis Thanks for your patience...I see that now. You're absolutely right that fixes it.

MGatner · 2021-10-12T12:24:09Z

Are we sure this is safe across different OSes? See e.g. Michal's comment above.

lf-uraku-yuki · 2021-10-12T14:08:41Z

@kenjis Fix conflicts by weekend.

kenjis · 2021-10-13T00:39:38Z

@lf-uraku-yuki Thank you for your cooperation!

nmolinos · 2021-10-13T00:44:35Z

Are we sure this is safe across different OSes? See e.g. Michal's comment above.

Since this change would effectively cause sessions to be removed after 24 minutes of inactivity (for the PHP.ini default of 1440 in some cases -- when sessionExpiration is set to 0), this should definitely be made more apparent. Possibly adding a note to the configuration file where sessionExpiration is set, in addition the ChangeLog as mentioned by Michal.

kenjis · 2021-10-13T01:00:57Z

When you set $sessionExpiration 0.

Before:

session.gc_maxlifetime = 0
session.cookie_lifetime = 0

After:

session.gc_maxlifetime = 1440 (PHP default)
session.cookie_lifetime = 0

Session Configuration

See https://www.php.net/manual/en/session.configuration.php

Server Side

session.gc_maxlifetime default: 1440
specifies the number of seconds after which data will be seen as 'garbage' and potentially cleaned up. Garbage collection may occur during session start (depending on session.gc_probability and session.gc_divisor).

session.gc_probability default: 1
in conjunction with session.gc_divisor is used to manage probability that the gc (garbage collection) routine is started.

session.gc_divisor default: 100
coupled with session.gc_probability defines the probability that the gc (garbage collection) process is started on every session initialization. The probability is calculated by using gc_probability/gc_divisor, e.g. 1/100 means there is a 1% chance that the GC process starts on each request.

Client Side

session.cookie_lifetime default: 0
specifies the lifetime of the cookie in seconds which is sent to the browser. The value 0 means "until the browser is closed."

kenjis · 2021-10-13T02:15:00Z

@MGatner @michalsn

As far as I remember setting 0 will work just fine on Debian and will stop PHP gc entirely.

I've confirmed Debian 10.11's behavior.
$sessionExpiration = 0 does not stop PHP gc.

Procedure

Build docker environment and installs CI 4.1.4.

$ git clone [email protected]:kenjis/docker-codeigniter-apache.git
$ cd docker-codeigniter-apache
$ make create-project

Check Debian version.

$ make web
docker-compose exec web bash
root@df110741382e:/work/backend# cat /etc/debian_version 
10.11

Update Home controller.

<?php

namespace App\Controllers;

use Config\Services;

class Home extends BaseController
{
    public function index()
    {
        //ini_set('session.gc_divisor', (string) 1);

        $session = Services::session();
    }
}

Create session files.

$ curl http://localhost/
$ curl http://localhost/
$ curl http://localhost/
$ curl http://localhost/
$ curl http://localhost/

$ ls -l writable/session/
total 56
-rw-------  1 kenji  staff   91 10 13 11:09 ci_session3318f2d7835906ce4fbbdea234eafaef8bc6877a
-rw-------  1 kenji  staff   91 10 13 11:09 ci_session5716d271e8d185a3fbc919f106cf3b0b4ef4abd5
-rw-------  1 kenji  staff   91 10 13 11:09 ci_session675da4983f476cf258b5b3ba9f8fe7d77443ff04
-rw-------  1 kenji  staff   91 10 13 11:09 ci_sessionaaba50ceee3c236fa456359cc97950a67d7f899d
-rw-------  1 kenji  staff   91 10 13 11:09 ci_sessionbabc6451df73ca53c87d496b49b287aa3db3a0d9
-rwxr-xr-x  1 kenji  staff  131 10 13 10:54 index.html

Set session.gc_divisor 1.

<?php

namespace App\Controllers;

use Config\Services;

class Home extends BaseController
{
    public function index()
    {
        ini_set('session.gc_divisor', (string) 1);

        $session = Services::session();
    }
}

Set $sessionExpiration 0.

--- a/backend/app/Config/App.php
+++ b/backend/app/Config/App.php
@@ -174,7 +174,7 @@ class App extends BaseConfig
      *
      * @var int
      */
-    public $sessionExpiration = 7200;
+    public $sessionExpiration = 0;
 
     /**
      * --------------------------------------------------------------------------

Run Home controller.

$ curl http://localhost/

$ ls -l writable/session/
total 8
-rwxr-xr-x  1 kenji  staff  131 10 13 10:54 index.html

kenjis · 2021-10-13T02:21:14Z

@MGatner @michalsn @nmolinos
session.gc_maxlifetime = 0 does not stop the GC. It sees all sessions as 'garbage' and potentially cleaned up.
So this PR does not shortens the lifetime of Sessions, just lengthens them.
See #4744 (comment)

But when a user set $sessionExpiration 0 (it means until the browser is closed),
the PHP default 1440 seconds may not be enough.

So it is better to add a note that when you set $sessionExpiration 0, you may want to change
session.gc_maxlifetime more than 1440.

michalsn · 2021-10-13T05:27:18Z

It seems like everything is working. So I'm okay with the changes. Thank you for taking your time to investigate @kenjis !

MGatner · 2021-10-14T11:55:22Z

Yes, stellar investigation! Puts my mind at ease. Is the comment you wrote above above appropriate for our User Guide? We would also need an entry in the Change Log it sounds like.

…ocument.

lf-uraku-yuki · 2021-10-16T08:16:07Z

The signature and committer email address do not match because the user specified at the time of commit was incorrect. I'm sorry .

MGatner

Looks good to me! Whoever merges please squash.

user_guide_src/source/libraries/sessions.rst

kenjis · 2021-10-17T00:58:18Z

@lf-uraku-yuki Thank your for your contribution!

Next time, if possible, please use git rebase instead of git merge to update your branch.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#pushing-your-branch

fix codeigniter4#4169 GC processing issues when session lifetime is 0

de7acf8

paulbalandan assigned michalsn and MGatner and unassigned michalsn and MGatner May 26, 2021

paulbalandan requested review from MGatner and michalsn May 26, 2021 08:46

MGatner approved these changes May 26, 2021

View reviewed changes

michalsn reviewed May 26, 2021

View reviewed changes

MGatner mentioned this pull request Jun 15, 2021

Bug: Session Library #4807

Closed

kenjis requested changes Oct 11, 2021

View reviewed changes

lf-uraku-yuki added 2 commits October 16, 2021 15:49

Merge branch 'develop' into fix_session_gc

6af314f

Added description about the structure of the session library of the d…

5acdf4f

…ocument.

MGatner approved these changes Oct 16, 2021

View reviewed changes

paulbalandan reviewed Oct 16, 2021

View reviewed changes

Apply suggestions from code review

c3e980c

paulbalandan changed the title ~~fix #4169 GC processing issues when session lifetime is 0~~ Fix GC issue when session lifetime is set to 0 Oct 16, 2021

paulbalandan approved these changes Oct 16, 2021

View reviewed changes

paulbalandan merged commit f3424a5 into codeigniter4:develop Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GC issue when session lifetime is set to 0 #4744

Fix GC issue when session lifetime is set to 0 #4744

lf-uraku-yuki commented May 26, 2021 •

edited by paulbalandan

Loading

MGatner commented May 26, 2021

MGatner commented May 26, 2021

lf-uraku-yuki commented May 26, 2021

lf-uraku-yuki commented May 26, 2021

MGatner left a comment

michalsn left a comment

MGatner commented May 26, 2021

nmolinos commented Oct 3, 2021 •

edited by kenjis

Loading

kenjis commented Oct 11, 2021

kenjis left a comment

kenjis commented Oct 11, 2021

kenjis commented Oct 11, 2021

nmolinos commented Oct 11, 2021

kenjis commented Oct 11, 2021

nmolinos commented Oct 11, 2021

MGatner commented Oct 12, 2021

lf-uraku-yuki commented Oct 12, 2021

kenjis commented Oct 13, 2021

nmolinos commented Oct 13, 2021

kenjis commented Oct 13, 2021 •

edited

Loading

kenjis commented Oct 13, 2021

kenjis commented Oct 13, 2021

michalsn commented Oct 13, 2021

MGatner commented Oct 14, 2021

lf-uraku-yuki commented Oct 16, 2021

MGatner left a comment

kenjis commented Oct 17, 2021

Fix GC issue when session lifetime is set to 0 #4744

Fix GC issue when session lifetime is set to 0 #4744

Conversation

lf-uraku-yuki commented May 26, 2021 • edited by paulbalandan Loading

MGatner commented May 26, 2021

MGatner commented May 26, 2021

lf-uraku-yuki commented May 26, 2021

lf-uraku-yuki commented May 26, 2021

MGatner left a comment

Choose a reason for hiding this comment

michalsn left a comment

Choose a reason for hiding this comment

MGatner commented May 26, 2021

nmolinos commented Oct 3, 2021 • edited by kenjis Loading

kenjis commented Oct 11, 2021

kenjis left a comment

Choose a reason for hiding this comment

kenjis commented Oct 11, 2021

kenjis commented Oct 11, 2021

nmolinos commented Oct 11, 2021

kenjis commented Oct 11, 2021

nmolinos commented Oct 11, 2021

MGatner commented Oct 12, 2021

lf-uraku-yuki commented Oct 12, 2021

kenjis commented Oct 13, 2021

nmolinos commented Oct 13, 2021

kenjis commented Oct 13, 2021 • edited Loading

Session Configuration

Server Side

Client Side

kenjis commented Oct 13, 2021

Procedure

kenjis commented Oct 13, 2021

michalsn commented Oct 13, 2021

MGatner commented Oct 14, 2021

lf-uraku-yuki commented Oct 16, 2021

MGatner left a comment

Choose a reason for hiding this comment

kenjis commented Oct 17, 2021

lf-uraku-yuki commented May 26, 2021 •

edited by paulbalandan

Loading

nmolinos commented Oct 3, 2021 •

edited by kenjis

Loading

kenjis commented Oct 13, 2021 •

edited

Loading