Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Feature request #78555. Add the option to disable dblink cleanup #4712

wants to merge 2 commits into from

Conversation

eriklundin
Copy link
Contributor

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

@krakjoe
Copy link
Member

krakjoe commented Sep 30, 2019

@nikic can you forward this ping to someone who can review this please, I can't remember the correct human ...

@nikic
Copy link
Member

nikic commented Sep 30, 2019

Perhaps @andreyhristov or @johannes?

@nikic
Copy link
Member

nikic commented Sep 30, 2019

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.

@andreyhristov
Copy link

Hmm,
thinking about this function can be abused for leaving connections open / leaking memory. Especially problematic on shared hosting. Thus, I think this function should be limited to CLI/embed SAPIs and not available with web servers.

@eriklundin
Copy link
Contributor Author

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.

@andreyhristov
Copy link

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.

@eriklundin
Copy link
Contributor Author

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?

@nikic
Copy link
Member

nikic commented Oct 11, 2019

As this came up just now on the ML ... a possibly better alternative here would be to provide something like pcntl_exit(), which directly invokes libc _exit() and bypasses PHP shutdown entirely. For the fork use-case, that seems more robust than trying to suppress individual shutdown behaviors.

@eriklundin
Copy link
Contributor Author

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.

@nikic
Copy link
Member

nikic commented Oct 14, 2019

@eriklundin Shutdown functions still get run on fatal error, so doing something like

register_shutdown_function(function() {
    // SIGKILL or pcntl_exit()
}):

should be robust?

@eriklundin
Copy link
Contributor Author

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().

@ramsey
Copy link
Member

ramsey commented May 31, 2022

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!

@eriklundin
Copy link
Contributor Author

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

#8697

@cmb69
Copy link
Member

cmb69 commented Jun 7, 2022

Closing in favor of PR #8697.

@cmb69 cmb69 closed this Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants