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

Don't set session.gc_probality=1, instead read value from config #15274

Closed
Jakhotiya opened this issue Dec 16, 2019 · 9 comments · Fixed by #16480
Closed

Don't set session.gc_probality=1, instead read value from config #15274

Jakhotiya opened this issue Dec 16, 2019 · 9 comments · Fixed by #16480
Labels
Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.

Comments

@Jakhotiya
Copy link

@ini_set('session.gc_probability', 1);

As in the code snippet above setting
session.gc_probability=1 in general causes problems for different environments. For example our session handler is redis and session save_path for webserver is /var/run/redis.sock
In this case automatic garbage cleanup will attempt to cleanup a directory to which it does not have access to, For example, opendir(/var/lib/php/session) failed: permission denied fails in my case.
As a result I see errors like
ERROR Piwik\Session[2019-12-12 09:14:06 UTC] [2267b] Unable to start session
Occasionally which are frequent but hard to reproduce.

Many systems do not give permission to session directory because they have some kind of cron to periodically clear expired session files. Just the php recommendation
When you are using redis for php sessions there is no need for garbage collection since TTL will take care of removing old sessions. Hence session.gc_probability should be zero.

Now, this concerned me because link to configure redis for matomo sessions

Should we

  1. update code base with something like
@ini_set(Config->getInstance()->General['session_gc_probability']);

2.) Just update the FAQ link with help..(I don't know what help. How do we tell matomo not to force gc_probability to 1? Is there an event? Can I do Dependency injection?)

3.) leave it alone for now

@tsteur
Copy link
Member

tsteur commented Dec 16, 2019

@Jakhotiya this FAQ might be bit outdated. We're using DB sessions by default which are working nicely in a load balanced environment and I'm thinking we will be deprecating support for this. There shouldn't really be a need for using Redis sessions over DB.

@Jakhotiya
Copy link
Author

@tsteur You would have this problem even with DB right? Because when session_start() will still try to cleanup sessions file system directory. ps_files_cleanup_dir is called before session start. Am I missing something?

@tsteur
Copy link
Member

tsteur commented Dec 19, 2019

@diosmosis do you maybe know more about it? If it doesn't have an effect on the DB, we could probably remove it?

@diosmosis
Copy link
Member

It affects when session GC is called, and the session_set_save_handler function requires a $gc function be supplied. So with 0 for the db session handler, I think old sessions would never be deleted. I don't see why it would result in deleting files though. Perhaps this is something strange that the redis handler is doing in it's GC.

@Jakhotiya
Copy link
Author

@diosmosis @tsteur it has nothing to do with redis handler. it has everything to do with "ps_files_cleanup_dir" which is called when we call session_start() function. Strangely there is no official php doc for "ps_files_cleanup_dir";

See if this link helps.
https://bugs.php.net/bug.php?id=20720

The bug I'm reporting will creep up with many different php frameworks that set gc_probability=1

This thing happens even when I'm not using redis session handler. I see this on our production environment when GC is triggered.

@diosmosis
Copy link
Member

@tsteur we could add the INI config setting pretty easily, what do you think?

@Jakhotiya
Copy link
Author

@diosmosis I can send a pull request.

@tsteur
Copy link
Member

tsteur commented Dec 20, 2019

Sounds good 👍

@diosmosis
Copy link
Member

@Jakhotiya feel free to create a PR 👍

@mattab mattab added the Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. label Jan 21, 2020
tsteur pushed a commit that referenced this issue Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants