POC: lightweight subprocess isolation via pcntl_fork() #5751

staabm · 2024-03-16T07:11:52Z

still work in progress..

codecov · 2024-03-16T07:16:28Z

Codecov Report

Attention: Patch coverage is 46.91358% with 86 lines in your changes are missing coverage. Please review.

Project coverage is 89.78%. Comparing base (53eebaa) to head (3628c7b).

Files	Patch %	Lines
src/Util/PHP/PcntlFork.php	0.00%	82 Missing ⚠️
src/Framework/TestRunner.php	40.00%	3 Missing ⚠️
src/Framework/TestBuilder.php	96.55%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5751      +/-   ##
============================================
- Coverage     90.13%   89.78%   -0.36%     
- Complexity     6631     6679      +48     
============================================
  Files           697      701       +4     
  Lines         20026    20175     +149     
============================================
+ Hits          18051    18114      +63     
- Misses         1975     2061      +86

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sebastianbergmann · 2024-03-16T07:58:56Z

Keep in mind that isolation is more important than performance. I mention this because I do not know how much global state (included files, declared constants, loaded classes, global variables, ...) carries over from the parent process into the child process when forking is used.

staabm · 2024-03-17T06:43:07Z

In my understanding the goal of process isolation is to isolate the tests from each other.

Is it also the goal to isolate phpunit from the test?

sebastianbergmann · 2024-03-17T07:03:59Z

Is it also the goal to isolate phpunit from the test?

No.

staabm · 2024-03-17T19:08:08Z

perf comparison this PR fork@1dc7b80:

➜  phpunit git:(fork) ✗ php ./phpunit tests/_files/TestWithClassLevelIsolationAttributes.php
PHPUnit 11.1-gadccd7efc by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.17
Configuration: /Users/staabm/workspace/phpunit/phpunit.xml

R                                                                   1 / 1 (100%)

Time: 00:00.006, Memory: 8.00 MB

There was 1 risky test:

1) PHPUnit\TestFixture\TestBuilder\TestWithClassLevelIsolationAttributes::testOne
This test did not perform any assertions

/Users/staabm/workspace/phpunit/tests/_files/TestWithClassLevelIsolationAttributes.php:24

OK, but there were issues!
Tests: 1, Assertions: 0, Risky: 1.

vs. main-branch@4303b9eb2

➜  phpunit git:(main) php ./phpunit tests/_files/TestWithClassLevelIsolationAttributes.php
PHPUnit 11.1-g4303b9eb2 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.17
Configuration: /Users/staabm/workspace/phpunit/phpunit.xml

R                                                                   1 / 1 (100%)

Time: 00:00.046, Memory: 8.00 MB

There was 1 risky test:

1) PHPUnit\TestFixture\TestBuilder\TestWithClassLevelIsolationAttributes::testOne
This test did not perform any assertions

/Users/staabm/workspace/phpunit/tests/_files/TestWithClassLevelIsolationAttributes.php:24

OK, but there were issues!
Tests: 1, Assertions: 0, Risky: 1.

POC looks 7-8x faster

staabm · 2024-03-17T19:54:45Z

src/Framework/TestRunner.php

+
+            $test->setInIsolation(true);
+            try {
+                $test->run();


I think I need to check $runEntireClass at this point and run only a single method. not sure how yet.
(in a similar way it is done in runInWorkerProcess())

MasonM

One problem with pcntl_fork() is database connections. If the parent process has a connection to MySQL/PostgreSQL/etc, the child process will inherit that resource. When the child exits, the connection will be automatically closed, which means subsequent children will receive connection errors. More details at https://www.php.net/manual/en/function.pcntl-fork.php#70721

staabm · 2024-03-22T18:48:32Z

If a test creates a resource which needs isolation, said test needs to run in isolation.

As soon as a single test runs in the main process with something which has sideeffects on other tests, things break. But thats not unique to forking. You have the same problem with worker processes, as soon as the test which requires isolation runs not in a worker

MasonM · 2024-03-22T19:23:21Z

@staabm You're correct that the current process-based isolation mechanism (which uses proc_open()) also has problems with side effects under certain circumstances, but database connections aren't one of them. Database connections aren't inherited when proc_open() is used, but they will with this change, which means it's backwards-incompatible. Creating database connections in the main process before running tests with @runInSeparateProcess is a common pattern in the codebases I work with.

You could prevent breaking backwards compatibility by making this opt-in using a new config option or attribute, but this is a pretty big footgun.

staabm · 2024-03-22T19:33:16Z

I see. Thanks for mentioning. I guess if this PR propsal will land, we will definitely need a opt-in until we get some decent real world battle test experience

MasonM · 2024-03-22T19:40:58Z

No problem. I'm happy to run any proposed changes on our codebase, which make heavy use of process isolation and would greatly benefit from any performance benefits. I came across this while doing my own research on optimizing such tests, but unfortunately I haven't found anything that helps without major caveats.

staabm · 2024-03-29T09:12:33Z

I have just added annotation support, so one can opt-in to fork based isolation via attribute.

example test which uses fork based isolation:

<?php declare(strict_types=1);
/*
 * This file is part of PHPUnit.
 *
 * (c) Sebastian Bergmann <sebastian@phpunit.de>
 *
 * For the full copyright and license information, please view the LICENSE
 * file that was distributed with this source code.
 */
namespace PHPUnit\TestFixture\Metadata\Attribute;

use PHPUnit\Framework\Attributes\RunClassInSeparateProcess;
use PHPUnit\Framework\Attributes\RunInSeparateProcess;
use PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses;
use PHPUnit\Framework\TestCase;

#[RunClassInSeparateProcess(true)]
#[RunTestsInSeparateProcesses]
final class ProcessIsolationForkedTest extends TestCase
{
    #[RunInSeparateProcess]
    public function testOne(): void
    {
    }
}

the true in #[RunClassInSeparateProcess(true)] indicates that fork should be used instead of a separate worker.
that way we can easily ship the feature and don't break existing assumptions of worker based isolation per default.
(Currently I don't plan to also add phpdoc annotation support, as I read this will be dropped in the next major)

tbh I don't like bool attribute parameters, but I found it would be consistent with other attributes.
IMO something like #[RunClassInSeparateProcess("forkIfPossible")] would be easier to read.

this PR is also missing some more end-to-end tests, but I wanted to discuss the UX and design before going into full test-coverage

I am open for feedback

staabm · 2024-04-14T08:08:12Z

@sebastianbergmann any feedback on #5751 (comment) ?

sebastianbergmann · 2024-04-14T08:13:27Z

Currently I don't plan to also add phpdoc annotation support, as I read this will be dropped in the next major

No new annotations are added since PHPUnit 10 came out.

the true in #[RunClassInSeparateProcess(true)] indicates that fork should be used

I am not sure whether I like the boolean flag, I think I would prefer a separate attribute for this. Don't worry, I would implement the attribute and metadata-related changes after merging this.

I think we should resolve #5230 / #5233 before working on this, though. What do you think?

sebastianbergmann · 2024-04-18T12:59:02Z

@staabm Heads-up: I have cleaned up the code a bit:

Introduced PhpJob value object that encapsulates all relevant information about a job
Introduced DefaultPhpJobRunner class for running PhpJobs using the tried-and-true implementation using proc_open() etc.
Migrated existing code to use DefaultPhpJobRunner

DefaultPhpJobRunner implements the PhpJobRunner interface. For now, this interface is @internal and no mechanism is in place to configuration an implementation to use instead of DefaultPhpJobRunner. This might change in the future if and when we identify a need for this (sooner if you tell me that you would like to implement your pcntl_fork() based approach that is the topic of this PR as a PhpJobRunner implementation).

staabm · 2024-04-18T13:34:02Z

thanks for the groundwork. I am planning to bring this pctnl based process isolation into phpunit, if we find a way which is acceptable for you. I think it can be really useful to have a faster isolation alternative.

sebastianbergmann · 2024-04-18T13:54:47Z

thanks for the groundwork. I am planning to bring this pctnl based process isolation into phpunit, if we find a way which is acceptable for you. I think it can be really useful to have a faster isolation alternative.

Okay, then I'll try to find time ASAP to make this pluggable. I already how to do it, just need to do it.

staabm · 2024-04-18T14:01:54Z

thanks. no hurry.

pcntl based isolation is not a blocker for me... its just one item on my bucket list to make the world a bit better :-)

sebastianbergmann · 2024-04-19T06:12:24Z

As of 191477f, PhpJobRunnerRegistry::set() can be used to configure a custom PhpJobRunner implementation.

staabm · 2024-04-21T08:31:00Z

thx for your work. I tried rebasing the PR, but I am not sure yet, whether we have all pieces in the JobRunner to implement a pcntl variant.

I need a way to run the job in the current process like I did in

https://github.com/sebastianbergmann/phpunit/pull/5751/files#diff-2843ca5c5620803be3ae210b4cd8f4f74eac269d29f3fef343ce212a3aed05f1R92-R119

but it seems PhpJob alone does not provide enough context so I can run the TestCase. Maybe I missed something..?

sebastianbergmann · 2024-04-28T06:58:28Z

Maybe I missed something?

I doubt that you are missing something. The PhpJob value object currently only encapsulates the information that the default implementation of PhpJobRunner needs. What exactly do you need?

staabm · 2024-05-04T05:34:30Z

in a PcntlJobRunner I would need to fork the current process.

after forking

in the main process I would need to wait for the forked worker process to end and report the result
in the child process I would need to run the test itself, report the result to the main process and kill the child

all the above - forking, main and child logic - would happen within the JobRunner.
currently I see no way to run the test-case from within a JobRunner.
I think therefore I need a reference to TestCase within my PcntlJobRunner.

sebastianbergmann · 2024-05-06T12:54:39Z

@staabm Would 6342542 help?

staabm force-pushed the fork branch from 7cd11eb to adccd7e Compare March 17, 2024 19:04

staabm commented Mar 17, 2024

View reviewed changes

staabm force-pushed the fork branch from 9a8e78c to f353d0b Compare March 17, 2024 20:28

clxmstaab force-pushed the fork branch from 3977652 to 32144c4 Compare March 18, 2024 09:19

sebastianbergmann added type/performance Issues related to resource consumption (time and memory) feature/process-isolation Issues related to running tests in separate PHP processes labels Mar 18, 2024

MasonM reviewed Mar 22, 2024

View reviewed changes

staabm force-pushed the fork branch 2 times, most recently from 8219df7 to a47ff12 Compare March 29, 2024 09:07

staabm force-pushed the fork branch from a47ff12 to 65344ad Compare March 30, 2024 11:55

staabm marked this pull request as ready for review April 1, 2024 13:52

staabm force-pushed the fork branch from 65344ad to 59b5b1d Compare April 2, 2024 11:36

staabm and others added 8 commits April 14, 2024 10:07

POC: lightweight subprocess isolation via pcntl_fork()

1845f0a

implement ipc

d3d05ee

Update TestRunner.php

dd8f873

types

43973e2

extract PcntlFork

7f1905b

cs

118605d

CI: add pcntl job

7adcdee

run unit tests also with pcntl

0758468

staabm added 2 commits April 14, 2024 10:07

added fork-if-possible annotation support

38dfa46

wire annotations

3628c7b

staabm force-pushed the fork branch from 59b5b1d to 3628c7b Compare April 14, 2024 08:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: lightweight subprocess isolation via pcntl_fork() #5751

POC: lightweight subprocess isolation via pcntl_fork() #5751

staabm commented Mar 16, 2024 •

edited

codecov bot commented Mar 16, 2024 •

edited

sebastianbergmann commented Mar 16, 2024

staabm commented Mar 17, 2024

sebastianbergmann commented Mar 17, 2024

staabm commented Mar 17, 2024 •

edited

staabm Mar 17, 2024

MasonM left a comment

staabm commented Mar 22, 2024

MasonM commented Mar 22, 2024

staabm commented Mar 22, 2024

MasonM commented Mar 22, 2024

staabm commented Mar 29, 2024 •

edited

staabm commented Apr 14, 2024

sebastianbergmann commented Apr 14, 2024

sebastianbergmann commented Apr 18, 2024 •

edited

staabm commented Apr 18, 2024

sebastianbergmann commented Apr 18, 2024

staabm commented Apr 18, 2024

sebastianbergmann commented Apr 19, 2024 •

edited

staabm commented Apr 21, 2024 •

edited

sebastianbergmann commented Apr 28, 2024

staabm commented May 4, 2024 •

edited

sebastianbergmann commented May 6, 2024 •

edited

POC: lightweight subprocess isolation via pcntl_fork() #5751

Are you sure you want to change the base?

POC: lightweight subprocess isolation via pcntl_fork() #5751

Conversation

staabm commented Mar 16, 2024 • edited

codecov bot commented Mar 16, 2024 • edited

Codecov Report

sebastianbergmann commented Mar 16, 2024

staabm commented Mar 17, 2024

sebastianbergmann commented Mar 17, 2024

staabm commented Mar 17, 2024 • edited

staabm Mar 17, 2024

Choose a reason for hiding this comment

MasonM left a comment

Choose a reason for hiding this comment

staabm commented Mar 22, 2024

MasonM commented Mar 22, 2024

staabm commented Mar 22, 2024

MasonM commented Mar 22, 2024

staabm commented Mar 29, 2024 • edited

staabm commented Apr 14, 2024

sebastianbergmann commented Apr 14, 2024

sebastianbergmann commented Apr 18, 2024 • edited

staabm commented Apr 18, 2024

sebastianbergmann commented Apr 18, 2024

staabm commented Apr 18, 2024

sebastianbergmann commented Apr 19, 2024 • edited

staabm commented Apr 21, 2024 • edited

sebastianbergmann commented Apr 28, 2024

staabm commented May 4, 2024 • edited

sebastianbergmann commented May 6, 2024 • edited

staabm commented Mar 16, 2024 •

edited

codecov bot commented Mar 16, 2024 •

edited

staabm commented Mar 17, 2024 •

edited

staabm commented Mar 29, 2024 •

edited

sebastianbergmann commented Apr 18, 2024 •

edited

sebastianbergmann commented Apr 19, 2024 •

edited

staabm commented Apr 21, 2024 •

edited

staabm commented May 4, 2024 •

edited

sebastianbergmann commented May 6, 2024 •

edited