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 #16448

Merged
merged 2 commits into from
Mar 25, 2024
Merged

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_warn

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

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Jun 18, 2023
@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (69a7d6d) 21.75% compared to head (b3b1df3) 21.68%.
Report is 28 commits behind head on 3.x.

Additional details and impacted files
@@             Coverage Diff              @@
##                3.x   #16448      +/-   ##
============================================
- Coverage     21.75%   21.68%   -0.07%     
- Complexity    10478    10496      +18     
============================================
  Files           561      561              
  Lines         31590    31703     +113     
============================================
+ Hits           6872     6875       +3     
- Misses        24718    24828     +110     

see 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Mark-H
Mark-H previously approved these changes Aug 4, 2023
@Mark-H Mark-H added this to the v3.1.0 milestone Aug 4, 2023
Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

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

I'd suggest following changes, it removes some unnecessary ifs and makes it slightly more readable.

*/
protected function _checkSessionsGarbageCollector()
{
$success = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$success = false;
$status = 'success';

Comment on lines 571 to 570
} else {
$success = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} else {
$success = true;
}
}

Comment on lines 569 to 570
$success = @ini_set('session.gc_probability', 1);
$success = $success !== false ? true : false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$success = @ini_set('session.gc_probability', 1);
$success = $success !== false ? true : false;
$status = @ini_set('session.gc_probability', 1) !== false ? 'success' : 'fail';

Comment on lines 576 to 580
if ($success) {
$this->warn('session_gc', '', $this->install->lexicon('test_session_gc_success', ['gc_probability' => $gc_probability, 'gc_divisor' => $gc_divisor]));
} else {
$this->warn('session_gc', '', $this->install->lexicon('test_session_gc_fail', ['gc_probability' => $gc_probability, 'gc_divisor' => $gc_divisor]));
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ($success) {
$this->warn('session_gc', '', $this->install->lexicon('test_session_gc_success', ['gc_probability' => $gc_probability, 'gc_divisor' => $gc_divisor]));
} else {
$this->warn('session_gc', '', $this->install->lexicon('test_session_gc_fail', ['gc_probability' => $gc_probability, 'gc_divisor' => $gc_divisor]));
}
$this->warn('session_gc', '', $this->install->lexicon("test_session_gc_$status", ['gc_probability' => $gc_probability, 'gc_divisor' => $gc_divisor]));

Copy link
Member

@theboxer theboxer left a comment

Choose a reason for hiding this comment

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

👍

@Ruslan-Aleev Ruslan-Aleev added the pr/ready-for-merging Pull request reviewed and tested and ready for merging. label Oct 13, 2023
@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

Do the lexicons need to be in both default.inc.php and test.inc.php?

@Ruslan-Aleev
Copy link
Collaborator Author

@Mark-H To be honest, I don’t know, I haven’t figured it out. Other keys are present in both files.

@opengeek opengeek merged commit 70cca64 into modxcms:3.x Mar 25, 2024
5 of 7 checks passed
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
@Ruslan-Aleev Ruslan-Aleev deleted the 3.x-fix-16275 branch March 25, 2024 15:40
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. pr/ready-for-merging Pull request reviewed and tested and ready for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants