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 (v8.0) #8569

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.

@morrisonlevi morrisonlevi force-pushed the stop-closing-cli-streams-8.0 branch 5 times, most recently from 82cf8fe to c166e12 Compare May 18, 2022 15:30
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 force-pushed the stop-closing-cli-streams-8.0 branch from c166e12 to 6d12aec Compare May 18, 2022 15:39
@morrisonlevi
Copy link
Contributor Author

So our test runner doesn't check for non-zero exit codes. These four tests are failing on the PHP-8.0 branch with non-zero exit codes:

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Bug #78376 (Incorrect preloading of constant static properties) [ext/opcache/tests/bug78376.phpt]
Bug #78175.2 (Preloading segfaults at preload time and at runtime) [ext/opcache/tests/bug78175_2.phpt]
Preloading inherited method with separated static vars [ext/opcache/tests/preload_method_static_vars.phpt]
Preload trait with static variables in method [ext/opcache/tests/preload_trait_static.phpt]
=====================================================================

They all print zend_mm_heap corrupted, but since stderr is closed at that time, it's not getting printed. All my patch does is allow us to see the message that it was already trying to print.

This shouldn't block this patch being merged, as it's merely uncovering a problem that already exists.

@morrisonlevi morrisonlevi changed the title Stop closing stderr and stdout streams (v8.0) [CLI] Stop closing standard streams (v8.0) 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 added a commit to arnaud-lb/php-src that referenced this pull request May 20, 2022
* PHP-8.0:
  XFAIL tests (phpGH-8588)
  Stop closing stderr and stdout streams (php#8569)
arnaud-lb added a commit to arnaud-lb/php-src that referenced this pull request May 20, 2022
* PHP-8.1:
  Stop closing stderr and stdout streams (php#8570)
  Revert "XFAIL tests (phpGH-8588)"
  XFAIL tests (phpGH-8588)
  Stop closing stderr and stdout streams (php#8569)
@arnaud-lb
Copy link
Member

Merged in fa78e17

@arnaud-lb arnaud-lb closed this May 20, 2022
@morrisonlevi morrisonlevi deleted the stop-closing-cli-streams-8.0 branch May 20, 2022 15:42
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
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.

2 participants