laravel / octane

Supercharge your Laravel application's performance.
https://laravel.com/docs/octane
MIT License
3.76k stars 294 forks source link

RoadRunner crashes with --watch, starting with version 2.9.3 #524

Closed ResuBaka closed 2 years ago

ResuBaka commented 2 years ago

Description:

When using file watch the artisan octane:start process crashes. As the RoadRunner cli writes infos to the stderr.

php artisan octane:start --watch  

   INFO  Server running…

  Local: http://127.0.0.1:8000 

  Press Ctrl+C to stop the server

   INFO  Application change detected. Restarting workers…

   RuntimeException 

  Cannot reload RoadRunner: 2022/05/20 08:07:16 resetting plugin: [http]

  at vendor/laravel/octane/src/RoadRunner/ServerProcessInspector.php:56
     52▕             'reset',
     53▕             '-o', "rpc.listen=tcp://$host:$rpcPort",
     54▕         ], base_path()))->start()->waitUntil(function ($type, $buffer) {
     55▕             if ($type === Process::ERR) {
  ➜  56▕                 throw new RuntimeException('Cannot reload RoadRunner: '.$buffer);
     57▕             }
     58▕ 
     59▕             return true;
     60▕         });

      +30 vendor frames 
  31  artisan:37
      Illuminate\Foundation\Console\Kernel::handle(Object(Symfony\Component\Console\Input\ArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))

Steps To Reproduce:

  1. Install RoadRunner 2.9.4
  2. start octane server with --watch
  3. Change a file so the server needs to be restarted
  4. Process ends with an error

Possible solution to fix:

What could be done to fix the issue is to execute the commands with the -s, --silent flag so the messages are not written.

driesvints commented 2 years ago

I indeed get the same thing when using the latest versions.

driesvints commented 2 years ago

@rustatian do you might have any ideas here?

rustatian commented 2 years ago

Hey, @driesvints 👋🏻. We changed some output messages starting from the v2.9.4, especially rr reset message. You may use --silent with rr reset like ./rr reset --silent to avoid any output.

rustatian commented 2 years ago

Also, --silent flag might be used with a serve, like ./rr serve [options] --silent to hide a startup message [INFO] RoadRunner ...

driesvints commented 2 years ago

Okay but won't that also hide actual errors?

driesvints commented 2 years ago

I mean, isn't the issue here that roadrunner cannot reload?

rustatian commented 2 years ago

./rr reset might show only dial errors. If some error occurs, it'll be in the ./rr serve logs, but not in the ./rr reset output.

I mean, isn't the issue here that roadrunner cannot reload?

It's hard to say because there are no errors/logs from the RR. I double-checked that without octane and everything works ok. I can also guess, that if an error occurred only on versions >= 2.9.4, the only thing that was changed in these versions is ./rr reset output message.

rustatian commented 2 years ago

Also, remember that RR writes all informational messages to the stderr. I'm not sure, what does this mean:

if ($type === Process::ERR) {

I can only guess (since I'm not a PHP dev), that this is some stderr check for the data in it.

ResuBaka commented 2 years ago

The issue is that the Symfony Process handles in pipes stderr output as an error and calls the callback. Which then has the $type as error.

So --silent option would fix it, as I would thing rr reset would return an error code to the console when it fails?

rustatian commented 2 years ago

I would thing rr reset would return an error code to the console when it fails?

Yeah, sure. --silent hide an output but not the process error code. So, this is entirely safe to use that flag for the CLI commands because actual RR errors will be in the logs.

driesvints commented 2 years ago

Okay cool. Can anyone send in a PR for that. Thanks for your help!

ResuBaka commented 2 years ago

Should we check the version to add the --silent option or just add it always for the reset?

As I would then create a PR for it.

rustatian commented 2 years ago

Or, I don't know if that's ok, you may filter these messages from the stderr. I mean, resetting plugin: [http] and plugin reset: [http]. Those messages are stable and won't be changed in the v2. If you still wanted to check the stderr.

rustatian commented 2 years ago

Should we check the version to add the --silent option or just add it always for the reset?

You may add it as the default. Previous versions will ignore this flag w/o any additional output.

ResuBaka commented 2 years ago

Filtering for the messages in octane does not work as we only get the first message ever as a callback and never the second.

So I will implement the --silent option.