Improve stack trace when running the tool with parallel runner

[FeBe95]

Feature request

Since version 3.57.0 we are running PHP CS Fixer in parallel with great success. Though, we've encountered an issue while using the parallel runner: The stack trace when running into errors is kind of useless in parallel mode. Using --sequential resolves this issue and returns a meaningful stack trace again.
It would be great to have a similar stack trace when using the parallel runner.

Please have a look at this issue for an exemplary error:

• simplified_null_return errors out with TypeError: Illegal offset type #8037

Stack trace when using sequential execution:

PhpCsFixer\Fixer\ReturnNotation\SimplifiedNullReturnFixer->needFixing()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Fixer\ReturnNotation\SimplifiedNullReturnFixer.php at line 71
PhpCsFixer\Fixer\ReturnNotation\SimplifiedNullReturnFixer->applyFix()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\AbstractFixer.php at line 75
PhpCsFixer\AbstractFixer->fix()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 422
PhpCsFixer\Runner\Runner->fixFile()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 363
PhpCsFixer\Runner\Runner->fixSequential()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 158
PhpCsFixer\Runner\Runner->fix()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Console\Command\FixCommand.php at line 338
PhpCsFixer\Console\Command\FixCommand->execute()
  in C:\<redacted>\vendor\symfony\console\Command\Command.php at line 326
[ ... ]

Stack trace when using parallel execution:

PhpCsFixer\Error\SourceExceptionFactory::fromArray()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 277
PhpCsFixer\Runner\Runner->PhpCsFixer\Runner\{closure}()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Parallel\Process.php at line 173
PhpCsFixer\Runner\Parallel\Process->PhpCsFixer\Runner\Parallel\{closure}()
  in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
Evenement\EventEmitter->emit()
  in C:\<redacted>\vendor\clue\ndjson-react\src\Decoder.php at line 139
Clue\React\NDJson\Decoder->handleData()
  in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
Evenement\EventEmitter->emit()
  in C:\<redacted>\vendor\react\stream\src\Util.php at line 71
React\Stream\Util::React\Stream\{closure}()
  in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
Evenement\EventEmitter->emit()
  in C:\<redacted>\vendor\react\stream\src\DuplexResourceStream.php at line 196
React\Stream\DuplexResourceStream->handleData()
  in C:\<redacted>\vendor\react\event-loop\src\StreamSelectLoop.php at line 246
React\EventLoop\StreamSelectLoop->waitForStreamActivity()
  in C:\<redacted>\vendor\react\event-loop\src\StreamSelectLoop.php at line 213
React\EventLoop\StreamSelectLoop->run()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 349
PhpCsFixer\Runner\Runner->fixParallel()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 157
PhpCsFixer\Runner\Runner->fix()
  in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Console\Command\FixCommand.php at line 338
PhpCsFixer\Console\Command\FixCommand->execute()
  in C:\<redacted>\vendor\symfony\console\Command\Command.php at line 326
[ ... ]

Link to Discussion where feature request was formed

No response

Contribution Checks

• I have verified if this feature request was already discussed
• I am familiar with "Feature or bug?"

[Wirone]

@FeBe95 is -vv not enough for you? In general, error handling in child processes is painful, as it's not possible to transmit exceptions (and their trace) from worker to main process in a sane way. I tried to serialise them etc, we ended with something in between.

[FeBe95]

I agree, transferring complex data like a whole stack trace between child/parent processes wouldn't be reasonable.

Though, with -vv I was left a bit clueless, since I had no idea where the TypeError (see linked issue) was actually coming from. I looked into our project's code, and couldn't find any errors, and that's when I started using -vvv.

Maybe, instead of passing the whole stack trace from child to parent, it could be possible to transfer the file name only. Provided that is it possible at all to detect the source of an exception from the parent process.

As an example, the output of the linked issue could then look something like this:

PhpCsFixer\Fixer\ReturnNotation\SimplifiedNullReturnFixer:101
[TypeError]
Illegal offset type

With this information we would at least know if the issue is caused in our project's code and where to look for debugging purposes.

[Wirone]

All I see in the linked issue is that you used -vvv --sequential which gives native exception from the main process. Parallel runner with -vv shows stack traces from both main, and child processes. Exception's type and message are transferred from the worker and used for throwing exception in the main process, so all the information should be there. Can you reproduce this issue once again and test it with -vv using parallel runner?

[FeBe95]

Sure thing, here you go:

Non-verbose

> .\vendor\bin\php-cs-fixer check test.php --using-cache=no --rules=simplified_null_return

PHP CS Fixer 3.57.1 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.2.12
Running analysis on 32 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from "C:\<redacted>\.php-cs-fixer.php".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Found 0 of 1 files that can be fixed in 0.917 seconds, 22.000 MB memory used

Files that were not fixed due to errors reported during fixing:
   1) C:\<redacted>\test.php

Verbose (-v)

> .\vendor\bin\php-cs-fixer check test.php --using-cache=no --rules=simplified_null_return -v

PHP CS Fixer 3.57.1 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.2.12
Running analysis on 32 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from "C:\<redacted>\.php-cs-fixer.php".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Found 0 of 1 files that can be fixed in 0.901 seconds, 22.000 MB memory used

Files that were not fixed due to errors reported during fixing:
   1) C:\<redacted>\test.php

Very verbose (-vv)

> .\vendor\bin\php-cs-fixer check test.php --using-cache=no --rules=simplified_null_return -vv

PHP CS Fixer 3.57.1 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.2.12
Running analysis on 32 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from "C:\<redacted>\.php-cs-fixer.php".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Found 0 of 1 files that can be fixed in 0.964 seconds, 22.000 MB memory used

Files that were not fixed due to errors reported during fixing:
   1) C:\<redacted>\test.php

                             
        [TypeError]          
        Illegal offset type  
                             

Debug (-vvv)

> .\vendor\bin\php-cs-fixer check test.php --using-cache=no --rules=simplified_null_return -vvv

PHP CS Fixer 3.57.1 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.2.12
Running analysis on 32 cores with 10 files per process.
Parallel runner is an experimental feature and may be unstable, use it at your own risk. Feedback highly appreciated!
Loaded config default from "C:\<redacted>\.php-cs-fixer.php".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


Found 0 of 1 files that can be fixed in 0.917 seconds, 22.000 MB memory used

Files that were not fixed due to errors reported during fixing:
   1) C:\<redacted>\test.php

                             
        [TypeError]          
        Illegal offset type  
                             

      PhpCsFixer\Error\SourceExceptionFactory::fromArray()
        in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 277
      PhpCsFixer\Runner\Runner->PhpCsFixer\Runner\{closure}()
        in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Parallel\Process.php at line 173
      PhpCsFixer\Runner\Parallel\Process->PhpCsFixer\Runner\Parallel\{closure}()
        in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
      Evenement\EventEmitter->emit()
        in C:\<redacted>\vendor\clue\ndjson-react\src\Decoder.php at line 139
      Clue\React\NDJson\Decoder->handleData()
        in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
      Evenement\EventEmitter->emit()
        in C:\<redacted>\vendor\react\stream\src\Util.php at line 71
      React\Stream\Util::React\Stream\{closure}()
        in C:\<redacted>\vendor\evenement\evenement\src\EventEmitterTrait.php at line 143
      Evenement\EventEmitter->emit()
        in C:\<redacted>\vendor\react\stream\src\DuplexResourceStream.php at line 196
      React\Stream\DuplexResourceStream->handleData()
        in C:\<redacted>\vendor\react\event-loop\src\StreamSelectLoop.php at line 246
      React\EventLoop\StreamSelectLoop->waitForStreamActivity()
        in C:\<redacted>\vendor\react\event-loop\src\StreamSelectLoop.php at line 213
      React\EventLoop\StreamSelectLoop->run()
        in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 349
      PhpCsFixer\Runner\Runner->fixParallel()
        in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Runner\Runner.php at line 157
      PhpCsFixer\Runner\Runner->fix()
        in C:\<redacted>\vendor\friendsofphp\php-cs-fixer\src\Console\Command\FixCommand.php at line 338
      PhpCsFixer\Console\Command\FixCommand->execute()
        in C:\<redacted>\vendor\symfony\console\Command\Command.php at line 326
      [ ... ]

[Wirone]

Ah OK, now I understand. It's not a worker error (unhandled), but a process error that is caught during the process, serialised on worker's side and reported to the main process, so the mentioned -vv won't work here as it's only about printing original stack trace for an unhandled errors that stop the process execution. We lose original stack trace for errors caught during analysis, and currently it's not possible to restore original stack trace for them. The current flow is:

• Main process spawns child processes
• each worker gets file chunks and analyses them
• if there's an error during the process, it's reported to error manager on worker's side, and then it's sent to the main process via socket
• main process receives ParallelAction::WORKER_RESULT payload and does SourceExceptionFactory::fromArray($error['source']), which basically tries to re-create exception/error using original class, also tries to restore file and line properties
• main process lists encountered errors at the end of the process

Even if we add original stack trace to the payload sent from the worker to the main process, we can't recreate it within recreated exception/error, it's just not possible in PHP. And since SourceExceptionFactory::fromArray() returns Throwable, we also cant extend the contract to have the access to the stack trace as a string. I already put an huge effort around it in #7777 (tried serialisation, WyriHaximus/php-json-throwable etc.), and it just requires changing the approach to error handling. We shouldn't rely on Throwable in PhpCsFixer\Error\Error, we should have custom DTO representing error, that would be fully serialisable and could be recreated in the main process from the payload sent by the worker. In order to have such information in the output, we need change how error manager works and what data it operates with.

@keradus are you open to such changes in the Error / ErrorManager?

[keradus]

If I understand you right, you want to replace:

-    public function __construct(int $type, string $filePath, ?\Throwable $source = null, array $appliedFixers = [], ?string $diff = null)
+    public function __construct(int $type, string $filePath, ?ErrorSourceDTO$source = null, array $appliedFixers = [], ?string $diff = null)

, where

class ErrorSourceDTO {
    var $msg, $code, $trace;
}

?

if so, I'm tentatively OK with it, but would need to see implementation draft.

I am also OK to have sth smaller, like a msg on parallel execution error, eg
to see details of the error, re-run the command with "--sequential -vvv"

[github-actions]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.