8000 Use destructor to close session (partially reverted #348) by supersmile2009 · Pull Request #412 · snc/SncRedisBundle · GitHub
[go: up one dir, main page]
Skip to content

Use destructor to close session (partially reverted #348) #412

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

Jump to bottom
Merged
merged 2 commits into from
Apr 18, 2018
Merged

Use destructor to close session (partially reverted #348) #412

merged 2 commits into from
Apr 18, 2018

Conversation

supersmile2009
Copy link
Contributor

Registering a shutdown function to close session in PR #348 was a great idea... However removing a destructor was a terrible one.

We have a pretty heavy test suite in our app and after updating to SncRedisBundle v2.1 (which included that PR) memory usage in it just skyrocketed, PHP started crashing running out of memory.

This PR adds destructor back while also keeping a shutdown function introduced in #348. It solves the memory issue we're facing.

@supersmile2009
Copy link
Contributor Author

Well, I re-checked it, and it seems that I looked at the wrong test run when opening this PR.
My PR doesn't solve the problem.
Reverting PR #348 solves issue completely.

I guess the problem is with registering shutdown function. When you boot many kernels in a big test suite it register this function every time, wasting up memory.

I think we need a config option to optionally enable/disable registering shutdown function. I'll update this PR tomorrow with such implementation.

@curry684
Copy link
Collaborator

Seems related to #400?

But isn't the root cause of this (and that) problem that you're actually trying to execute all those tests in the same process, which is something that PHP is really not meant to do given the whole existence of functions like register_shutdown_handler?

I'm curious whether your issues are solved if you run phpunit with --process-isolation=true, or add processIsolation="true" to the XML config file, or use annotations like @runInSeparateProcess. See also @runTestsInSeparateProcess on how to apply this to a single test class in the suite.

Keep in mind that we will likely not consider it wise to change/break perfectly fine code for web request handling scenarios just to facilitate unit tests, if the tests can be easily fixed by configuration or annotations that were invented for this precise purpose.

@curry684 curry684 mentioned this pull request Apr 12, 2018
Closed
@Seldaek
Copy link
Collaborator
Seldaek commented Apr 13, 2018 •
edited
Loading

We could maybe merge this and then enable the register_shutdown_function behavior only when locking is enabled. Without locking, a request timing out/failing probably shouldn't write the session data anyway. The problem when locking is that you end up with a session deadlock and block other requests for a while.

Then in your test you can at least disable locking to make sure you avoid the memory leak. Process isolation is a solution but it also makes tests super slow..

@curry684
Copy link
Collaborator

Possibly more practical approach: we could use framework.test setting to disable/enable some behaviors, like not setting shutdown handlers on one hand and aggressively closing open connections during kernel termination otherwise. Would be a matter of just compiling some extra event handlers in there.

@Seldaek
Copy link
Collaborator
Seldaek commented Apr 13, 2018

Yes.. or just making the register_shutdown_function optional with a constructor argument, then it's easy to toggle on/off. Because down the line this is a bad idea, it prevents writing app servers for example without leaking memory.

@curry684
Copy link
Collaborator

Quick fix for 2.1 could also be to introduce some static behaviors, pseudocode:

static $cleanup = [];
if (empty($cleanup)) {
  register_shutdown_handler(function() use ($cleanup) {
    foreach ($cleanup as $that) {
      $that->close();
    }
  });
}
$cleanup[] = $this;

@Seldaek
Copy link
Collaborator
Seldaek commented Apr 13, 2018

I don't know how much this helps, it'll still keep a handle to every instance created so that leaks all the same IMO.

@curry684
Copy link
Collaborator

Mmm true. So we should likely merge this and keep the framework.test approach in mind for a more structural solution later.

@supersmile2009
Copy link
Contributor Author

@curry684 Thanks for the tip about --process-isolation, I wasn't familiar with this option. It may work, but at the moment enabling it breaks a lot of out tests, so we'd rather use a patch in our composer.json for now to disable shutdown function.

As suggested by @Seldaek, register_shutdown_function should only be enabled when session locking is enabled.
For the moment apart from what you see in this PR, we're going to use a small patch adding this

        if (true === $locking) {
            register_shutdown_function(array($this, 'shutdown'));
        }

into RedisSessionHandler constructor.
This way it's possible to set snc_redis.session.locking: false in test config, which will solve the issue.
I'm not adding it into this PR since you already approved it, but If you think it's a good idea, I can push it to this branch.

@curry684
Copy link
Collaborator

enabling it breaks a lot of out tests

That indicates your tests depend on order of execution and/or other tests to succeed, which is considered bad practice. Interdependency should be handled with @depends annotations, making a chain of tests explicit.

I can push it to this branch

Fine with me, looks harmless enough and I'll merge it as it's a quick workaround for now.

@curry684
Copy link
Collaborator

For the record: holding off on merge until that addition is made.

@curry684
Copy link
Collaborator

Also #400 is a slightly different problem that we're not solving now. Could we do that in a practical way?

@curry684 curry684 merged commit ae3a607 into snc:2.1 Apr 18, 2018
@curry684
Copy link
Collaborator

Merged, thanks!

curry684 pushed a commit that referenced this pull request Apr 18, 2018
@supersmile2009 @curry684
Use destructor to close session (partially reverted #348) (#412)
b133296 
* Use destructor to shutdown session (partially reverted #348)

* Register shutdown function only if session locking is enabled

(cherry picked from commit ae3a607)
@curry684 curry684 mentioned this pull request Apr 19, 2018
Closed
curry684 added a commit that referenced this pull request Apr 19, 2018
@curry684
Remove destructor from session handler
ed56780 
Reported to cause performance issues, refs #417, reverts #412
@goetas
Copy link
Contributor
goetas commented May 20, 2020 •
edited
Loading

Wanted just to share here my experience with session locking and Redis.
File based sessions use the filesystem locking, so if PHP process that is holding the lock terminates abnormally, it will automatically release the lock. This is also true if the script reaches the max_execution_time and phpfpm decides to terminate the child (it is done sending a SIGTERM signal).
Unfortunately when using redis based locking, is fundamental that the script terminates gracefully, so the destructor gets called and release the lock. Unfortunately that does not happen, and some session are locked. The most common situation when this happens are max_execution_time limits.

As a solution we have used:

pcntl_async_signals(true);
pcntl_signal(SIGTERM, static function () {
    exit;
});

That installs a signal handler for the SIGTERM signal and uses exit. I this way when PHP-FPM sends the termination signal, a normal exit is executed, that will allow PHP to invoke all the session unlocking features.

This implementation has as side effect that the worker process does not terminate on SIGTERM anymore because exit in reality does not terminate the FPM worker, but just tells to PHP to stop the script execution.

Fortunately when PHP-FPM scales down the workers, it uses SIGQUIT, so is able to free memory.

Does anyone have an idea on how to handle this in a better way?

@B-Galati
Copy link
Collaborator

@goetas The Symfony Lock component may have fixed this problem a better way?

@goetas
Copy link
Contributor
goetas commented May 28, 2020

@B-Galati I do not think that is possible as locking issues are caused by phpfpm killing the process without calling destructors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@curry684 curry684 curry684 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@supersmile2009 @curry684 @Seldaek @goetas @B-Galati
0