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

Php 8.2 upgrade has caused an error #59

Closed
mhmdmousawi opened this issue Oct 12, 2023 · 18 comments
Closed

Php 8.2 upgrade has caused an error #59

mhmdmousawi opened this issue Oct 12, 2023 · 18 comments

Comments

@mhmdmousawi
Copy link

I have been using this library with php version 8.0 for a while.
And when I decided to upgrade the php version to 8.2 my end to end tests are now failing with the following error:

                                                                                                                        
[donatj\MockWebServer\Exceptions\ServerException] Failed to start server. Is something already running on port 5500?  
                                                                                                                        
#1  /var/www/vendor/donatj/mock-webserver/src/MockWebServer.php:85

I tried to debug the code to check if the server was running actually, It was not!

While debugging, I tried to dump $this->isRunning() in File /donatj/mock-webserver/src/MockWebServer.php:

dump($this->isRunning());
for( $i = 0; $i <= 20; $i++ ) {
	usleep(100000);
	$open = @fsockopen($this->host, $this->port);
	if( is_resource($open) ) {
		fclose($open);
		break;
	}
}
dump($this->isRunning());

This was actually returning false on the first dump and true on the second!!

Do you know how is that related to the php upgrade, and how we can solve that?

@donatj
Copy link
Owner

donatj commented Oct 12, 2023 via email

@mhmdmousawi
Copy link
Author

I am running my project on Docker image (ubuntu:20.04), with php 8.2 installed.

The test is failing when it is trying to start the mock server (on setup):

EndToEndTestCase.php

<?php

declare(strict_types=1);

namespace App\Tests\EndToEnd;

use App\Tests\Util\Server\MicroServiceServer;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;

abstract class EndToEndTestCase extends WebTestCase
{
    protected MicroServiceServer $microServiceServer;

    public function setUp(): void
    {
        $this->microServiceServer = (new MicroServiceServer())->start();
    }

    protected function tearDown(): void
    {
        $this->microServiceServer->stop();

        parent::tearDown();
    }
}

MicroServiceServer.php

<?php

namespace App\Tests\Util\Server;

use donatj\MockWebServer\MockWebServer;

class MicroServiceServer
{
    private MockWebServer $mockServer;

    public function __construct()
    {
        $this->mockServer = new MockWebServer(5500);

    }

    public function __destruct()
    {
        if ($this->mockServer->isRunning()) {
            $this->mockServer->stop();
        }
    }
}

@donatj
Copy link
Owner

donatj commented Oct 12, 2023

Do you have any sort of test parallelizers installed like Paratest?

@mhmdmousawi
Copy link
Author

Looks like I have robo-paracept as part of my composer.json

@donatj
Copy link
Owner

donatj commented Oct 12, 2023

That's probably the problem, you're ending up with multiple servers fighting over port 5500.

It depends on your setup, but a good option if 5500 isn't too hard coded would be letting MockWebServer find it's own port and then using that in your tests.

e.g.

$this->webserver = new MockWebServer();
$this->port = $this->webserver->getPort();

// then your test uses $port instead of 5500

@mhmdmousawi
Copy link
Author

Although I am not sure this would be a solution, as I am dealing with End to End test.

I have tried the approach suggested, and I still get the same error, but now with the new port allocated by the constructor!

[donatj\MockWebServer\Exceptions\ServerException] Failed to start server. Is something already running on port 34201? 

Does this library support php 8.2 version?

@donatj
Copy link
Owner

donatj commented Oct 13, 2023

PHP 8.2 is almost certainly not the root cause here. PHP 8.2 is CI'd against and is used by hundreds of projects including several of my own.

You can see here it accounts for 28% of our installs.

image

My money is on the parallel testing causing the issue. It's possible your old docker containers were only getting a single CPU core and weren't actually executing in parallel. There are a lot of variables here.

The mock-webserver though was not built for parallel execution, and has not been verified against it. Even when in autoport mode, best case with parallel testing would likely still be flakey depending on how the particular OS assigns ports.

Autoport mode operates by asking the OS for an open port on __construct, but does not reserve it until you actually call start() on the server. There is nothing to stop another mock webserver on another thread from grabbing the port in the interim between finding the port and starting the server on said port.

Is there any way on your part you would be able to disable parallel execution and see if that helps the problem?

@mhmdmousawi
Copy link
Author

mhmdmousawi commented Oct 13, 2023

I can assure that the problem is not caused by parallel testing! (In my project I am not using any parallel testing)
Downgrading from version 2.6.1 to 2.6.0 have solved the issue!

So it looks like there is an issue with the changes v2.6.0...v2.6.1

@donatj
Copy link
Owner

donatj commented Oct 16, 2023

Viewing the diff between 2.6.0 and 2.6.1, most of the changes are cosmetic in nature.

The most significant updates include the fact that stdout and stderr are now logged to separate temporary files, rather than the same file. Additionally, we've made a change to use php://stdin instead of STDIN for improved reliability.

Personally, I can't see how any of these modifications are causing the issue you're encountering. I'm unable to reproduce it myself. I encourage you to dig into code and see if you can identify the change that's causing the problem. If you do happen to find something, by all means let me know.

I'd also recommend considering an upgrade to v2.7.0. Keep in mind that versions 2.6.0 and 2.6.1 are almost a year old, and v2.6.2 addressed a pretty significant pathing bug.

@mhmdmousawi
Copy link
Author

I have tested all versions starting from 2.6.1 until 2.7.0, all were failing except for 2.6.0 and below!

I will check if I can dig a bit deeper to see what is the issue exactly!

@sigma-z
Copy link

sigma-z commented Oct 27, 2023

I've taken a look into this, too, myself. It does not have something to do with PHP 8,2. This is that change that causes the issue: c7bdd7a and can also experienced with PHP 8.1.
It seems to be something with the use of streams in the $descriptorSpec. If I change the STDOUT and STDERR with tmpfile() instead of using the files as streams, then I am able to start the mock server.

I use Docker image php:8.1-apache.

@donatj
Copy link
Owner

donatj commented Oct 27, 2023

@sigma-z Are you using any sort of test parallelizer packages?

@sigma-z / @mhmdmousawi Hmm… Can either of you build me a small example project that exhibits the problem? I am frustratingly unable to reproduce the issue. My company runs multiple 8.1 and 8.2 projects that use this and we have seen no issues.

@sigma-z
Copy link

sigma-z commented Oct 27, 2023

@donatj No parallelizer, but I can try to create a small example that shows issue.

@sigma-z
Copy link

sigma-z commented Oct 27, 2023

@donatj The good news is, it is not the fault of mock-webserver (at least in my environment). I was able to track down the issue and it is caused by the package dg/bypass-finals, that un- and re-registers the stream wrapper for files in vendor/dg/bypass-finals/src/BypassFinals.php:52 and this causes the error PHP Warning: proc_open(): Unable to copy file descriptor 6 (for pipe) into file descriptor 1: Bad file descriptor. Please check out my example for reproduction at https://github.com/sigma-z/mock-webserver-test.

I can imagine @mhmdmousawi is also using the package bg/bypass-finals. Can you confirm?

Hope, this was of help.

@donatj
Copy link
Owner

donatj commented Oct 29, 2023

@sigma-z That was a ton of help. dg/bypass-finals is doing some truly funky stuff overriding the files stream. I wouldn't put a lot of trust in it. I did however open a ticket with them based on your example.

I may have been able to work around it though. Could you try changing your composer.json version requirement to

    "donatj/mock-webserver": "dev-fix/db-bypass-finals-compat"

@mhmdmousawi While I suspect your issue is probably unrelated to @sigma-z 's, would you please try that test branch as well?

@sigma-z
Copy link

sigma-z commented Oct 30, 2023

@donatj I can confirm that your fix in "dev-fix/db-bypass-finals-compat" is working properly.

Thank you for picking it up so quickly.

@donatj
Copy link
Owner

donatj commented Nov 1, 2023

@sigma-z - You may have already seen, but I've pushed that as a patch to v2.7.1 and v2.6.3

@donatj
Copy link
Owner

donatj commented Nov 28, 2023

@mhmdmousawi I am going to close this. If you ever circle back though I'd be curious to know if this fix resolved your issue.

@donatj donatj closed this as completed Nov 28, 2023
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

No branches or pull requests

3 participants