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

php: fix for macOS fork error #137909

Closed
wants to merge 1 commit into from

Conversation

askdkc
Copy link
Contributor

@askdkc askdkc commented Jul 28, 2023

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

This PR is related to this Laravel Valet issue: laravel/valet#1433 and it provides fix by adding following to the php plist.

<key>EnvironmentVariables</key>
<dict>
	<key>OBJC_DISABLE_INITIALIZE_FORK_SAFETY</key>
	<string>YES</string>
</dict>

So far, php 8.2.8 has problems connecting to PostgreSQL and Mongodb using Laravel. I'm sure there are another hidden problems with this version. Adding this env value will prevent some problems from happening.

Update

Apparently my solution also works for people having problem with PHP gettext #137431

And It seems that the upstream PHP team won't be able to resolve this issue soon (php/php-src#11818 (comment)). Because this is more of a system-level issue than a PHP issue.

Until it somehow ideally gets fixed by php upstream, Homebrew adding this environment variable to its php startup script is really providing nice iced water for people in the living hell. I mean, it's usually Mac people using Homebrew. Linux has their own distoro's package manager. Using homebrew for linux is unlikely. Homebrew providing Mac only work-around to its package does sound like it should be.

@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. icu4c ICU use is a significant feature of the PR or issue labels Jul 28, 2023
@carlocab carlocab changed the title Fix For macOS fork error php: fix For macOS fork error Jul 28, 2023
@chenrui333 chenrui333 changed the title php: fix For macOS fork error php: fix for macOS fork error Jul 28, 2023
Signed-off-by: Rui Chen <rui@chenrui.dev>
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Jul 28, 2023
@SMillerDev
Copy link
Member

So what is the actual bug and had this been reported to upstream ?

@askdkc
Copy link
Contributor Author

askdkc commented Jul 29, 2023

@SMillerDev

Issue

The latest macOS update has begun terminating certain fork() processes under specific circumstances. This is evidenced by the following screenshots and error logs

255328277-612559bd-d64e-4231-a6c5-5a60c9ba703a

In the Nginx Error Log, we find multiple instances of "upstream prematurely closed connection" errors. The PHP Error Log indicates warnings about "__NSPlaceholderDate initialize" possibly running in another thread when fork() was invoked, and subsequently, child processes exiting prematurely.

  • Nginx Error Log
[error] 1957#0: *2 upstream prematurely closed connection
[error] 50639#0: *77 upstream prematurely closed connection
[error] 28667#0: *16 upstream prematurely closed connection
  • PHP Error Log
[27-Jul-2023 09:49:38] NOTICE: ready to handle connections
[27-Jul-2023 09:49:48] WARNING: [pool valet] child 54119 said into stderr: "objc[54119]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called."
[27-Jul-2023 09:49:48] WARNING: [pool valet] child 54119 said into stderr: "objc[54119]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug."
[27-Jul-2023 09:49:48] WARNING: [pool valet] child 54119 exited on signal 6 (SIGABRT) after 9.676681 seconds from start
[27-Jul-2023 09:49:48] NOTICE: [pool valet] child 54693 started
[27-Jul-2023 09:50:29] WARNING: [pool valet] child 54120 said into stderr: "objc[54120]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called."
[27-Jul-2023 09:50:29] WARNING: [pool valet] child 54120 said into stderr: "objc[54120]: +[__NSPlaceholderDate initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead. Set a breakpoint on objc_initializeAfterForkError to debug."
[27-Jul-2023 09:50:29] WARNING: [pool valet] child 54120 exited on signal 6 (SIGABRT) after 50.322856 seconds from start
[27-Jul-2023 09:50:29] NOTICE: [pool valet] child 54703 started

How to reproduce

Follow this step: laravel/valet#1433 (comment)

Proposed Solution (This PR):

Rails users faced a similar issue a few years ago (refer to rails/rails#38560) and resolved it by introducing the environment variable

export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES.

However, Laravel Valet differs as it uses PHP, which is launched via LaunchDaemon. This leaves no room for adding the environment variable except in the homebrew.mxcl.php.plist file that is generated by Brew.

The aim of this PR is to add the OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES line to homebrew.mxcl.php.plist during the installation process, thereby fixing the issue where processes are prematurely terminated.

@SMillerDev
Copy link
Member

The latest macOS update has begun terminating certain fork() processes under specific circumstances. This is evidenced by the following screenshots and error logs

And the PHP team should be made aware of that and investigate a fix. Without a bug report upstream I'm very hesitant to merge this.

@askdkc
Copy link
Contributor Author

askdkc commented Jul 29, 2023

@SMillerDev

I agree with you and see your point.

My bad, it took some time to report this to PHP upstream. I submitted the issue.

php/php-src#11818

It might take some time for them to fix this. Meanwhile, hopefully this PR gets merged and bringing back better life experience for those who are using macOS 🙏😉

@askdkc
Copy link
Contributor Author

askdkc commented Aug 1, 2023

@SMillerDev

I am glad to let you know that I have fulfilled all your requests 😊.

This PR is targeted specifically to assist PHP developers on macOS who may not initially comprehend the cause of these unexpected errors.

The proposed solution consists of a single line of code:

(environment_variables OBJC_DISABLE_INITIALIZE_FORK_SAFETY: "YES")

This is a simple workaround that can be easily reverted (by removing it) if necessary. It's worth noting that ideally, the PHP team can address this issue (php/php-src#11818) in the future. However, given the uncertainty regarding the timeframe of such a resolution, this workaround provides an immediate fix.

Additionally, considering the recent changes to macOS security behavior—such as the disabling of system preference changes through LaunchDaemons (which caused an issue when the max file descriptors on my macOS suddenly dropped to 256 following the 13.5 update)—it's possible we might need to prepare additional workarounds should such changes continue to occur.

I sincerely hope this PR can be merged promptly to assist developers currently grappling with this issue. Your feedback and any further suggestions are highly valued and appreciated.

@SMillerDev
Copy link
Member

So one thing I'm worried about is that it only fixes the situation for people who use brew services with MongoDB and PGSQL. If you use HTTPD, it's still broken. If you use the CLI it's still broken. It therefore doesn't really sound like a fix.

@askdkc
Copy link
Contributor Author

askdkc commented Aug 1, 2023

So one thing I'm worried about is that it only fixes the situation for people who use brew services with MongoDB and PGSQL. If you use HTTPD, it's still broken. If you use the CLI it's still broken. It therefore doesn't really sound like a fix.

I'm not sure HTTPD side. But using Laravel with php artisan serve command is working fine with this issue without using this fix.

Could you give me your "If you use the CLI it's still broken." comment's example? Maybe I can help.

@SMillerDev
Copy link
Member

If you trigger the same codepath using php -S 0.0.0.0:8080 ./index.php you still have the issue. There are a lot of PHP developers not using php artisan serve too.

@askdkc
Copy link
Contributor Author

askdkc commented Aug 1, 2023

If you trigger the same codepath using php -S 0.0.0.0:8080 ./index.php you still have the issue. There are a lot of PHP developers not using php artisan serve too.

Can you avoid this issue by just running it like this?

export OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES

php -S 0.0.0.0:8080 ./index.php

@carlocab
Copy link
Member

carlocab commented Aug 1, 2023

@SMillerDev
Copy link
Member

grpc/grpc#33281 has relevant issues too.

@askdkc
Copy link
Contributor Author

askdkc commented Aug 3, 2023

@SMillerDev @carlocab

I think I've done a pretty good job troubleshooting the issue here (#137431)

And It seems that the upstream PHP team won't be able to resolve this issue soon (php/php-src#11818 (comment))

Would you consider merging this PR in the meantime? We can always easily revert these environment settings after it gets fixed.

Please make the life of PHP developers on macOS easier.

@askdkc
Copy link
Contributor Author

askdkc commented Aug 7, 2023

@SMillerDev

I've worked with PHP upstream team, and tried thier solution php/php-src#11818 (comment) but did not work out.

As @devnexen mentioned in the comment:

Would you be able to build php from sources ? if yes could try the php/php-src#11895 to see if it fixes. If it does not, I m afraid, the homebrew's change would be the only resort.

Could you now merge this PR for the time being?
That will make a lot of PHP devs life on macOS easier.
We can always revisit it for further improvements in the future if needed, right?

@carlocab
Copy link
Member

carlocab commented Aug 7, 2023

As I mention at php/php-src#11818 (comment), I don't think merging this is a good idea.

I'll quote the important parts from the article I shared again here:

Before High Sierra: appearing to work most of the time, occasionally failing catastrophically.

Most of the time, you're lucky and no other threads are doing important stuff. The application will appear to work fine. There may still be corrupted state somewhere, but you don't notice it — at least not immediately.

If you're unlucky then the application crashes or freezes in mysterious ways. In theory it can even accidentally wipe your hard drive.

Takeaway: the problem has always been there, even before High Sierra and even on Linux. It's just that High Sierra has chosen to fail fast instead of silently allowing corruption.

Enabling this disables safety features implemented by Apple, and that's not something we're going to foist on unsuspecting users.

As mentioned in php/php-src#11818 (comment),

I'm open to incorporating a workaround for Homebrew installations of PHP, but I'm just not convinced that setting OBJC_DISABLE_INITIALIZE_FORK_SAFETY is appropriate or safe to do, especially for users who are unaware that we've adopted this workaround.

Thus, closing this. If you're able to find a different, safer, workaround, feel free to open a different pull request for that.

@carlocab carlocab closed this Aug 7, 2023
dentarg added a commit to Starkast/wikimum that referenced this pull request Aug 17, 2023
We're crashing again (in macOS) since
Homebrew/homebrew-core#132976

This has occured before, from https://bugs.ruby-lang.org/issues/16239

> This is a fatal interaction between the PostgreSQL 12 client libraries
> and the GSS implementation provided by macOS. This is being tracked in
> the pg gem at ged/ruby-pg#311

More in ged/ruby-pg#311 (comment)

Docs on PGGSSENCMODE
https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-GSSENCMODE

OBJC_DISABLE_INITIALIZE_FORK_SAFETY=YES has been a workaround in the
past (puma/puma#1421) but it shouldn't be used
without care: Homebrew/homebrew-core#137909 (comment)
@github-actions github-actions bot added the outdated PR was locked due to age label Sep 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
icu4c ICU use is a significant feature of the PR or issue outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants