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

Automatically disable worker mode in local environments if workers aren't specified explicitly #932

Draft
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

Radiergummi
Copy link

This PR updates the FrankenPHP configuration to disable worker mode if the application is running in the local environment and the worker count is not specified explicitly (or set to auto).

The problem

FrankenPHP's Worker mode keeps the application in memory, which does wonders for performance when running in production. Locally, however, it causes problems with XDebug, and does not reflect file changes immediately. The solution to the latter is configuring Octane to restart the worker after every request, thereby defeating most of the advantages of an application server in the first place.

The solution

By only enabling worker mode if the application is running in a non-local environment, it will be served in a CGI-like manner, i.e. what the built-in PHP server (used in artisan serve) does—and exactly what you'd expect to happen: XDebug just works, and changes to files are reflected immediately.
In production, worker mode will be enabled and all the benefits that come with it apply. We use the setup as provided in this PR in our production application and don't face any issues with it.

This solves dunglas/frankenphp#931, and probably solves #928 too.

@dunglas
Copy link
Contributor

dunglas commented Jul 24, 2024

Could we detect if XDebug is installed and only disable the worker mode if it is?

It isn't by default and the worker mode improves performance even in dev mode.

@Radiergummi
Copy link
Author

Could we detect if XDebug is installed and only disable the worker mode if it is?

We could do ini_get('xdebug.mode') !== 'off', or possibly check APP_DEBUG in addition to the existing conditions?

It isn't by default and the worker mode improves performance even in dev mode.

Still, that would require either using watch mode (which in turn demands node and chokidar, which makes setup more complex in e.g. Docker) or limiting the server to MAX_REQUESTS=1, which isn't too good for performance either.

But @dunglas I respect if you want to approach this differently, and will close the PR—this is your domain.

@dunglas
Copy link
Contributor

dunglas commented Jul 25, 2024

@Radiergummi a problem with this approach is that Xdebug can be disabled (or enabled) for the CLI SAPI (which, generally, isn't using FrankenPHP), but enabled (or disabled) for the FrankenPHP SAPI. It's rare when using Docker, but more common when not using it.

Maybe would we store a tiny script detecting if Xdebug is enabled and run frankenphp php-cli the-detection-script.php to be more reliable?

@driesvints driesvints marked this pull request as draft July 25, 2024 09:01
@driesvints
Copy link
Member

We'll need to fix the tests before continuing.

…ankenPHP worker mode to avoid performance issues.

See the discussion with Kévin Dunglas in the PR discussion for the reasoning: laravel#932

Essentially, disabling worker mode comes with performance drawbacks, but it's still the only sensible way to use FrankenPHP with XDebug.
By automatically turning off worker mode if
 a) the amount of workers isn't set explicitly,
 b) we're running locally, and
 c) XDebug is not installed, or it's mode is set to "off",
we get both optimal performance and XDebug working out of the box.
@Radiergummi
Copy link
Author

@dunglas sorry for only getting back to this now, things were a bit tense on my end lately.

I just added a script as per your recommendation: Now, worker mode will be disabled if

  1. the amount of workers isn't set explicitly,
  2. we're running locally, and
  3. XDebug is not installed, or its mode is set to "off".

That should work as discussed earlier, no?

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.

3 participants