8000 Connections not properly closed on Kernel shutdown · Issue #400 · snc/SncRedisBundle · GitHub
[go: up one dir, main page]
Skip to content

Connections not properly closed on Kernel shutdown #400

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
Closed
applike-ss opened this issue Mar 22, 2018 · 10 comments
Closed

Connections not properly closed on Kernel shutdown #400

applike-ss opened this issue Mar 22, 2018 · 10 comments
Labels
enhancement Improves existing functionality

Comments

@applike-ss
Copy link

I experience propblems because of too many open connections during unit tests.
These connection are at half from ongr which doesn't close the connection on kernel shutdown => Garbage collection doesn't free it.

The used PHP Version is 7.1.12
Bundle is 2.0.6
@curry684
Copy link
Collaborator
curry684 commented Apr 6, 2018

Could you elaborate a bit more on the specific issue you are facing?

@curry684 curry684 added the needs-clarification Issue is not clear to maintainers label Apr 6, 2018
@curry684
Copy link
Collaborator
curry684 commented Apr 9, 2018

If I understand correctly => if you have a ton of functional tests instantiating rebooted kernels, and therefore reinstantiating the Redis instances, they will start failing in the end indicating Redis has run out of sockets or something?

@curry684 curry684 mentioned this issue Apr 12, 2018
Merged
@curry684
Copy link
Collaborator

@applike-ss I've just typed a bit in a related report at #412 (comment). As said there I'm not all too excited about changing or extending perfectly fine code just to facilitate test suites if it is easier and more correct to fix the test suite instead.

What is your take on that?

@applike-ss
Copy link
Author

@curry684 i do understand your point, but it is not the test case/suite that creates the problem.
In my Opinion, whenever a Bundle is shut down, all connections that this bundle initiated should be properly closed instead of relying on php to close them.
Doctrine is a good example for this behaviour. When it shuts down, it gathers all entity managers and clears their state. Afterwards it gathers all connections and closes them.
I believe, it shouldn't break anything anyway, since the connection wouldn't be used after a bundle shutdown anyway.

@applike-ss
Copy link
Author

You are right, having lots of tests using redis, we are facing an issue with connection count increasing to the maximum of the machine running the tests.

@curry684 curry684 added enhancement Improves existing functionality and removed needs-clarification Issue is not clear to maintainers labels Apr 17, 2018
@supersmile2009 supersmile2009 mentioned this issue Apr 18, 2018
Merged
@supersmile2009
Copy link
Contributor

@applike-ss check out #415 - should be a solution you're looking for.

@applike-ss
Copy link
Author

@supersmile2009 thanks a lot. Looking forward for it to be merged.

@curry684
Copy link
Collaborator
curry684 commented May 7, 2018

Fixed in 2.x by #415 but keeping this issue open to better investigate how to do this in 3.0 without making a service needlessly public.

@supersmile2009
Copy link
Contributor
supersmile2009 commented 8000 May 10, 2018

Like @dkarlovi mentioned on #415 it is possible to create env-specific service for test environment and declare it public instead of making public the original service.
However I'm not sure if there's anything else except unit tests that's using Kernel's shutdown feature. Method's docBlock says "This method is mainly useful when doing functional testing." But I'm particularly concerned about things like PPM - I haven't had a chance to try it yet, so I don't know whether it uses shutdown.

If shutdown is indeed only a unit tests thing, than a public service prefixed by test. and auto generated in a compiler pass is a perfect solution.
However if it's not the only case of shutdown usage, then we should probably also add a bool config option like snc_redis.session.close_on_sutdown. (default - false)
This way public service will be generated in test env + when this option is set to true, if someone needs it not just in test environment.

@dkarlovi
Copy link
dkarlovi commented May 10, 2018 •
edited
Loading

@supersmile2009 if you wish to support PPM, the official way to do it is to tag the service with kernel.reset.

ostrolucky added a commit that referenced this issue Oct 23, 2021
@ostrolucky
#400 Utilize kernel.reset to ensure connections have been closed in t…
5562d27 
…ests
ostrolucky added a commit that referenced this issue Oct 23, 2021
@ostrolucky
#400 Utilize kernel.reset to ensure connections have been closed in t…
a258c3b 
…ests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Improves existing functionality
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@dkarlovi @ostrolucky @curry684 @supersmile2009 @applike-ss
0