Skip to content
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.

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

Add session garbage collector check #16287

Closed
wants to merge 2 commits into from
Closed

Add session garbage collector check #16287

wants to merge 2 commits into from

Conversation

Ruslan-Aleev
Copy link
Collaborator

What does it do?

It often happens that the modx_session table grows uncontrollably (I have seen up to 5 gigabytes), which sometimes leads to complete non-operation of both the site and the server.

I added session garbage collector check in installer.

gc_installer

Why is it needed?

That there were no problems with overflow of the table modx_session.

How to test

Make the installation from scratch by changing the session.gc_probability, session.gc_divisor parameters.

Related issue(s)/PR(s)

#16275

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Oct 18, 2022
@Ruslan-Aleev Ruslan-Aleev added this to the v3.0.2 milestone Oct 18, 2022
@Mark-H
Copy link
Collaborator

Mark-H commented Oct 18, 2022

Does this prevent running the install or is this just a warning at the end after install completed?

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Oct 18, 2022

@Mark-H In this case, the installation will not continue - fail(). But we can do it through a warning - warn():

gc_installer_fail

gc_installer_warn

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Oct 18, 2022

There is also the question of whether it is necessary to check ONLY by session.gc_probability, or to add the check session.gc_divisor > 100. Because, for example, on my current hosting session.gc_divisor is 1000 000, which also causes the session table to overflow.

@Mark-H
Copy link
Collaborator

Mark-H commented Oct 18, 2022

Lowering to warning would be much better in my opinion. While yes it can cause issues to not have GC running, it shouldn't have to stop someone from running MODX entirely. There are non-server-configuration fixes for it.

SiteDash has a remote session GC feature that's on by default for example.

Or people could run a simple plugin like this to replicate a typical 1-in-1000 trigger:

if (random_int(1, 1000) === 1) {
    session_gc();
}

@Ruslan-Aleev
Copy link
Collaborator Author

Yes, I will indicate through a warning, this will be enough.
Question: what are the recommended values to specify for session.gc_probability and session.gc_divisor (1 and 100)?

@Mark-H
Copy link
Collaborator

Mark-H commented Oct 18, 2022

Question: what are the recommended values to specify for session.gc_probability and session.gc_divisor (1 and 100)?

That depends entirely on your server and traffic. You don't want it running too often, as it has a performance penalty, but you also don't want it to run too rarely as then it doesn't seem to work and will take a very long time to run when it finally does run.

Last I checked, MODX Cloud has it set to 1 in 1000, but as some of my dev sites are very low-traffic, that still has sessions from months ago. But if you run a production site that gets tens of thousands of hits per day, that might cause it to run too often. I think having it run once to twice a day is a good target in general. That means a probability of 1 and a divisor equal to your daily (php-served) requests.

In SiteDash's default setting, it runs the gc when there are sessions in the database older than 4 weeks. With a 1-week default session lifetime, that means it ends up running roughly every 3 weeks for sites that don't have GC or have a much too high divisor for the traffic it gets.

When changing SiteDash to it's "enabled" option (meant for when you know GC doesn't run on the server), it has a self-optimizing interval. I think it starts at 2 days. Then every time it runs, it either makes the interval longer (if it cleared less than 150 sessions) or shorter (if it cleared more than 500 sessions). And then applies a hard limit of at least 12 hours and at most 30 days.

@Ruslan-Aleev
Copy link
Collaborator Author

Ruslan-Aleev commented Oct 18, 2022

Maybe we should just print warn() anyway, without checking the conditions for the parameters? Because even if session.gc_probability = 1, overflow can still happen... And if we print pass(), then the issue will be unnoticed.
And in the message, specify detailed information and current parameter values.

@Mark-H
Copy link
Collaborator

Mark-H commented Oct 18, 2022

Yeah I'm okay with the warning. Maybe it's better to focus this on education, I'll make a note to write a new page in the docs about this issue and possible ways to go about it.

@opengeek opengeek modified the milestones: v3.0.2, v3.0.3 Nov 16, 2022
@opengeek opengeek modified the milestones: v3.0.3, v3.1.0 Jan 17, 2023
@Ruslan-Aleev Ruslan-Aleev deleted the 3.x-fix-16275 branch January 26, 2023 20:55
opengeek pushed a commit that referenced this pull request Mar 25, 2024
### What does it do?
It often happens that the `modx_session` table grows uncontrollably (I
have seen up to 5 gigabytes), which sometimes leads to complete
non-operation of both the site and the server.

I added session garbage collector check in installer.


![gc_installer_warn](https://user-images.githubusercontent.com/12523676/196445992-6b161c2c-519e-4cc4-bbe4-5ee67e60d150.png)

### Why is it needed?
That there were no problems with overflow of the table `modx_session`.

### How to test
Make the installation from scratch by changing the
`session.gc_probability, session.gc_divisor` parameters.

### Related issue(s)/PR(s)
#16287
#16275
opengeek pushed a commit that referenced this pull request Mar 25, 2024
### What does it do?
It often happens that the `modx_session` table grows uncontrollably (I
have seen up to 5 gigabytes), which sometimes leads to complete
non-operation of both the site and the server.

I added session garbage collector check in installer.


![gc_installer_warn](https://user-images.githubusercontent.com/12523676/196445992-6b161c2c-519e-4cc4-bbe4-5ee67e60d150.png)

### Why is it needed?
That there were no problems with overflow of the table `modx_session`.

### How to test
Make the installation from scratch by changing the
`session.gc_probability, session.gc_divisor` parameters.

### Related issue(s)/PR(s)
#16287
#16275
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core area-setup cla-signed CLA confirmed for contributors to this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants