Skip to content

[cache] Cache unchanged files on --dry-run #3834

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[cache] Cache unchanged files on --dry-run #3834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b487f05
[cache] Cache unchanged files on --dry-run
dorrogeray May 14, 2023
60b85e9
[cache] Simplify implementation
dorrogeray May 14, 2023
7d71fe5
[cache] Clear files cached by worker process request if that request …
dorrogeray May 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/Parallel/Application/ParallelFileProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
use React\EventLoop\StreamSelectLoop;
use React\Socket\ConnectionInterface;
use React\Socket\TcpServer;
use Rector\Caching\Detector\ChangedFilesDetector;
use Rector\Core\Configuration\Option;
use Rector\Core\Configuration\Parameter\ParameterProvider;
use Rector\Core\Console\Command\ProcessCommand;
Expand All @@ -24,6 +25,7 @@
use Symplify\EasyParallel\Enum\Content;
use Symplify\EasyParallel\Enum\ReactCommand;
use Symplify\EasyParallel\Enum\ReactEvent;
use Symplify\EasyParallel\ValueObject\ChildProcessTimeoutException;
use Symplify\EasyParallel\ValueObject\ParallelProcess;
use Symplify\EasyParallel\ValueObject\ProcessPool;
use Symplify\EasyParallel\ValueObject\Schedule;
Expand All @@ -46,7 +48,8 @@ final class ParallelFileProcessor

public function __construct(
private readonly WorkerCommandLineFactory $workerCommandLineFactory,
private readonly ParameterProvider $parameterProvider
private readonly ParameterProvider $parameterProvider,
private readonly ChangedFilesDetector $changedFilesDetector,
) {
}

Expand Down Expand Up @@ -125,6 +128,18 @@ public function process(
++$systemErrorsCount;
$reachedSystemErrorsCountLimit = true;
$this->processPool->quitAll();

// This sleep has to be here, because event though we have called $this->processPool->quitAll(),
// it takes some time for the child processes to actually die, and if we would delete the offending cache
// files right away, they could still write them "back" before they die
sleep(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am interesting with this sleep(1); part, I can found a usecase in the past which CTRL+C sometime still keep running other process in the background, and the file keep changing, especially when running from IDE.

Could you cherry pick this part into separate PR? Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cherrypicked into #4280

if ($throwable instanceof ChildProcessTimeoutException) {
$context = $throwable->getContext();

foreach ($context[Bridge::FILES] as $file) {
$this->changedFilesDetector->invalidateFile($file);
}
}
};

$timeoutInSeconds = $this->parameterProvider->provideIntParameter(Option::PARALLEL_JOB_TIMEOUT_IN_SECONDS);
Expand Down
2 changes: 1 addition & 1 deletion packages/Parallel/WorkerRunner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function run(Encoder $encoder, Decoder $decoder, Configuration $configura

if ($errorAndFileDiffs[Bridge::SYSTEM_ERRORS] !== []) {
$this->invalidateFile($file);
} elseif (! $configuration->isDryRun()) {
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this require e2e test for:

  • after multiple --dry-run keep show the diff, run apply changes to still apply the change.

For note: I think if you want current e2e timeout be deleted, that need to be replaced with "real crash use case", eg: can create a custom rule in e2e that must be crash, eg: echo dump($If_->else) node which the else still null so that will always crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samsonasik I have opened a new PR Cache unchanged files on dry run v2, it seems that all the tests are passing now. It looks like that this rework of the timeout test fixed the issue. Additionaly, I have updated the e2e_consecutive_changes.yaml to include two dry runs, covering the test case you mentioned. Please let me know if this is sufficient for the PR to get merged, thanks!

$this->changedFilesDetector->cacheFileWithDependencies($file->getFilePath());
}
} catch (Throwable $throwable) {
Expand Down