Add PHPProcessManager test for request queuing and instance reuse#3233
Add PHPProcessManager test for request queuing and instance reuse#3233
Conversation
Properly dispose PHP instances after each test to prevent memory accumulation during test runs.
This test covers the Bad Gateway issue that was addressed in #3219.
| mgr = new PHPProcessManager({ | ||
| phpFactory, | ||
| maxPhpInstances: 2, | ||
| timeout: 200, |
There was a problem hiding this comment.
Should this be set more generously so that it realistically never hits due to scheduling delays in CI when under high load?
Additionally, adding the intent to timeout values (here in this test and in other tests) when they are not under test, might help in making the intent clearer.
There was a problem hiding this comment.
Should this be set more generously so that it realistically never hits due to scheduling delays in CI when under high load?
I think we can't do that, because that's what this is testing—previously, when the scheduling was incorrect, this could've taken a bit more time than strictly necessary. We need to ensure that we meet the timings quite precisely.
To do that in a better way, I added vi.useFakeTimers(), and even lowered the timeout further. I also fixed the docs a bit to explain that the timeout actually represents "max. time in the queue" rather than queue + request duration.
There was a problem hiding this comment.
Pull request overview
Adds a regression test to ensure PHPProcessManager queues concurrent requests when instances are exhausted and reuses instances as they’re released (related to the Bad Gateway fix in #3219).
Changes:
- Introduces
afterEachcleanup to dispose the process manager between tests. - Updates existing tests to reuse a shared
mgrvariable. - Adds a new concurrency/queuing test that simulates 6 concurrent “requests” with a max of 2 PHP instances.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| simulateRequest(), | ||
| ]); | ||
|
|
||
| // All 6 requests should complete successfully within the 200ms timeout. |
There was a problem hiding this comment.
should we check if it's 6x 200 response code?
There was a problem hiding this comment.
@adamziel This is "requests" as in "acquirePHPInstance() and hold it for 50ms" requests. There is no HTTP response, not even a PHP code execution, it's only about scheduling—whether the timeout is met and whether there the correct number of PHP instances is acquired.
|
I addressed the PR feedback. It should be ready for a final check. |
ashfame
left a comment
ashfame
left a comment
There was a problem hiding this comment.
Tiny nitpicks, feel free to merge afterwards.
| mgr = new PHPProcessManager({ | ||
| phpFactory, | ||
| maxPhpInstances: 2, | ||
| timeout: 110, |
There was a problem hiding this comment.
Let's add a note on why we are trying to add buffer to the needed 100ms timeout? I noticed test fails with 100 but passes with 101, so I guess we are just waiting for fake timer to do the next tick? Is that the reason you had in mind too or something else?
There was a problem hiding this comment.
@ashfame Yes, that's how it works, but I think it's explained right above this call, isn't it?
There was a problem hiding this comment.
Wasn't apparent to me on why we need 10ms buffer. I thought 100ms should have been sufficient, which made me run the test myself with diff timeout values. But totally fine to not give too much mind to my comment.
There was a problem hiding this comment.
@ashfame Oh, OK, I'll add something like // 100ms for 4 requests + 10ms extra to start the remaining 2.
3 participants
Motivation for the change, related issues
This PR adds a regression test for the Bad Gateway fix implemented in #3219. The test ensures that the
PHPProcessManagercorrectly queues requests when all PHP instances are busy and properly reuses instances once they become available.Implementation details
The test simulates 6 concurrent requests with only 2 PHP instances available, each request taking 50ms to complete. With proper queuing and instance reuse, all 6 requests should complete within the 200ms timeout (3 batches × 50ms = 150ms).
The test pre-boots both PHP instances before starting the concurrent requests to eliminate instance spawning time from the timing calculations.
Testing Instructions (or ideally a Blueprint)
CI should be green. The new test can be run using: