-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Feature request #78555. Add the option to disable dblink cleanup #4712
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
Conversation
@nikic can you forward this ping to someone who can review this please, I can't remember the correct human ... |
Perhaps @andreyhristov or @johannes? |
In any case, it would be good to have a test for the new function together with pcntl_fork. I suspect that this disables a bit too much of the cleanup, e.g. the actual freeing of the resource should still be happening, otherwise you're doing to see warnings in debug builds. |
Hmm, |
The problem is not that it will leave memory allocated. The kernel will cleanup after the process when it dies. The problem is that mysql_close() ends the session within the connection. If there was just a close() on the FD there would be no problem. In any other situation, a proper cleanup is prefered, but when running forks it's a problem. The same problem exists in automatic cleanups from openssl resources running SSL_shutdown() on a socket. But that's another issue. |
The allocated memory will be cleaned up when the process finishes - right. If a multithreaded SAPI is used it might take some time. Thus, I think is an attack vector, which is only controllable with cgroups. This is why I mean : should be available only with CLI, which is not multithreaded. PHP is so popular, because it has a good sandbox. A sandbox the hosters rely onto. |
Limiting this function to CLI might be a good thing. I see no reason to have it enabled for web services. But the same goes for most functions in php-process. Abusing the server with memory limits etc. is not that hard to do if you have the functions available. What would be a sufficient way to only enable this when running from cli? |
As this came up just now on the ML ... a possibly better alternative here would be to provide something like |
Sending SIGKILL to the current process is possible today but it's a dirtier solution. One example is fatal errors. While you might be able to skip the cleanups on a normal exit. Unexpected fatal errors in the child process would still perform the cleanup? Being able to individually turn off cleanup for specific resources is better in every way. The perfect solution would be to be able to turn off dtor for all resources and classes individually. Not just mysqli but in a more generic way. This would make a bigger impact then adding a new function to mysqli, so that's why I didn't implement this. |
@eriklundin Shutdown functions still get run on fatal error, so doing something like
should be robust? |
You would also need to check if its the child or parent process in the shutdown handler if you wouldnt register a new one specific for the child. Theres a lot of workarounds that needs to be programmed just to avoid one extra function. Even then its just a workaround. Normally a process wouldnt need SIGKILL on a normal exit. And php’s cleanup on all other objects might be wanted. I would argue that giving the developer more control is not a bad thing. If you dont use this function, everything works exacly as before. But if you want more control you have the tools needed. Even if you disable the automatic cleanup on a specific object you can still use mysqli_close(). |
What is the status of this PR? It looks like it's still waiting for a review. If this PR is still relevant, please update it to target the master branch. Thanks! |
Yes it's till relevant. I've made a new PR that's targeting the master branch |
Closing in favor of PR #8697. |
When building daemon processes with php that requires the parent process to create a persistent database connection. The cleanup of the child process closes the parents database connection. A way to disable this cleanup on a object basis would make php better for daemon projects. This problem applies to other file descriptors as-well but the most problematic is the mysqli dblink resource.
Feature request url: https://bugs.php.net/bug.php?id=78555