Skip to content
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

[CLI] Stop closing standard streams #8571

Closed

Conversation

morrisonlevi
Copy link
Contributor

Extensions may (and do) write to stderr in mshutdown and similar. In
the best case, with the stderr stream closed, it's just swallowed.

However, some libraries will do things like try to detect color, and
these will outright fail and cause an error path to be taken.

See also #8569 and #8570.

@morrisonlevi morrisonlevi force-pushed the stop-closing-cli-streams-8.2 branch 2 times, most recently from e8eb980 to b9003e8 Compare May 16, 2022 23:25
Extensions may (and do) write to stderr in mshutdown and similar. In
the best case, with the stderr stream closed, it's just swallowed.

However, some libraries will do things like try to detect color, and
these will outright fail and cause an error path to be taken.
@morrisonlevi morrisonlevi changed the title Stop closing stderr and stdout streams [CLI] Stop closing standard streams May 19, 2022
arnaud-lb pushed a commit to arnaud-lb/php-src that referenced this pull request May 20, 2022
Extensions may (and do) write to stderr in mshutdown and similar. In
the best case, with the stderr stream closed, it's just swallowed.

However, some libraries will do things like try to detect color, and
these will outright fail and cause an error path to be taken.
@arnaud-lb
Copy link
Member

Thank you for these changes @morrisonlevi

Merged in c53c3e2

@arnaud-lb arnaud-lb closed this May 20, 2022
@morrisonlevi morrisonlevi deleted the stop-closing-cli-streams-8.2 branch May 20, 2022 15:42
@moonlitbugs
Copy link

This PR has just appeared in PHP-8.1.7, and has left me with a small farm of non-booting servers. 👎

A few lines of code will probably be worth several thousand words, so let me give that a whirl:
A system-init script as called from /sbin/init might look like:

#!/bin/bash
{ start_db
  start_php_code
} 2>&1 | tee /var/log/local-startup.log

And in in /usr/bin/start_php_code we have something like this:

#!/usr/bin/php
<?php
if (cli_args_are_ok() == false) {
  fputs(STDERR, "Bad args, aborting.\n");
  die(2);
}
// OK to run in background.
if (pcntl_fork() > 0) die(0); // Parent process.
echo "Backgrounding ourselves.  GoodBye.\n";
fclose(STDIN); fclose(STDOUT); fclose(STDERR);
chdir("/");
posix_setsid();
if (pcntl_fork() > 0) die(0); // Parent of double-fork.
// We are the child of the double-fork.  Run forever or until SIGTERM, etc.
for (;;) {
  // Main loop goes here...
}
?>

With php-8.1.7, the shell script that copies output to a log file never returns, because the start_php_code daemon leaves stdin/stdout/stderr open, even though fclose(STDOUT) etc falsely claimed to have succeeded.

I have edited my php-8.1.7 tarball, replacing sapi/cli/php_cli.c with the version from 8.1.6; this workaround fixes this regression.

dixyes added a commit to dixyes/phpmicro that referenced this pull request Jun 16, 2022
dixyes added a commit to dixyes/phpmicro that referenced this pull request Jun 16, 2022
dixyes added a commit to dixyes/phpmicro that referenced this pull request Jun 16, 2022
@iluuu1994
Copy link
Member

@morrisonlevi Can you comment on the above? @moonlitbugs Can you create a new issue?

@bukka
Copy link
Member

bukka commented Jun 20, 2022

I had a look on this and the problem is obviously that this prevented user to close those streams explicitly which is a regression so we need to revert this from lower branches. I'm not sure if we should even treat this as a bug as it's kind of excepted that that the stream might get closed and the extension should not relay on it being open.

That said I think we could make the live for extensions better by not to close it on non explicit closing (just as a feature in master). I think potentially better solution would be to have a different flag in streams that would prevent just closing the streams on resource dtor. I created an initial not properly tested changes that potentially not handle some edge cases yet (e.g. enclosed steams) in #8833 . I might also need to introduce a different option to add to fclose if this proves not to work correctly. But it should be doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants