-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Allow to not close stream on rscr dtor in php cli sapi #8833
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
@cmb69 yes agreed this should go to master only and we should revert the previous changes in 8.0 and 8.1 before the rc is tagged. I will need to do a bit more testing first and look to the possible edge cases but should be hopefully ready in the next few weeks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moonlitbugs
commented
Jul 6, 2022
@arnaud-lb , The test cited in #8827 only checks whether STDERR can be closed from within PHP. The STDERR global shows itself closed. But that is not the problem. The problem is that, when STDERR is closed, file descriptor 2 is open. Ditto STDIN/STDOUT.
As I noted in #8827, php-8.1.8 leaves /dev/fd/{0,1,2} open post-fclose(), despite the NEWS
claiming otherwise.
fds242
commented
Jul 6, 2022
The test cited in #8827 only checks whether STDERR can be closed from within PHP.
I fail to see what your issue is with the simple test I provided, since it continues to demonstrate the issue's existence in PHP 8.1.8, just as well as it did in 8.1.7, and also correctly pointing out every version which functioned correctly in the past. It cares not what the fclose() call returns, and instead tests if it php://stderr can be immediately reopened after the close attempt. Which it can, in the broken versions.
In any case, you're welcome to provide a better test.
I chose to test STDERR since what else could be reasonably tested in both a .phpt test and on ev4l.org? You don't control stdin; and stdout should not be messed with, since if you are able to successfully close it, you have also lost the ability for the test to output any results. As the bug has always equally affected all three std handles by design, seeing if the issue exists for stderr ought to be enough in this case.
moonlitbugs
commented
Jul 6, 2022
I fail to see what your issue is with the simple test I provided, since it continues to demonstrate the issue's existence in PHP 8.1.8, just as well as it did in 8.1.7
Ah, then I spoke too soon, my bad. I wrote that, as I remember that closing STDERR succeeds, var_dump() confirms, etc., yet the file descriptor itself is still open. Attempts at reference would return false, or throw an error. I was thinking that an external check of the file descriptor (system("lsof ...")
) would be necessary. But I can see now that fopen("php://stderr")
could work.
df033e3
to
945bd53
Compare
I just realised that I haven't pushed that test so pushed now. It's still not completely ready as I need to check some edge cases.
4de8207
to
d543ced
Compare
d543ced
to
d922298
Compare
d922298
to
0a4a55f
Compare
Uh oh!
There was an error while loading. Please reload this page.
This change allows not closing stream handle when resource dtor is being done. Fixes #8827