Add pooling and request limiting to prevent memory leaks #110

seanmorris · 2023-09-14T02:04:34Z

What is this PR doing?

Prevents PHP's memory leaks from causing the system to stop working once memory has been exhausted. It follows in the footsteps of systems like PHP-FPM by limiting the number of requests a PHP "thread" can handle before it is discarded and restarted, using Pooling behavior in packages/wp-now/src/start-server.ts.
~~--inspect and --inspect-brk switches have been added to allow the debugging of nodejs based WASM scripts in chrome dev tools.~~
~~--trace-warnings, --trace-uncaught, and --trace-exit flag have also been added to aid in debugging.~~

~~Struck~~ lines moved to #132

What problem is it solving?

PHP will eventually hit a memory leak when handling many requests one after another.

How is the problem addressed?

This PR adds behavior to track the number of requests a given PHP instance has handled, and discards it after a certain amount. A pool of PHP instances can be maintained so a fresh instance is always available, while discarded instances are replaced.

Testing Instructions

in `wordpress-playground`

This requires the latest development version of wordpress-playground, so we'll need to check out the trunk branch and link some packages first:

git checkout trunk
nvm use
npm install
npm run build

Then, switch to the dist/packages/playground/blueprints/ directory and link the package:

cd dist/packages/playground/blueprints/
npm link

In another terminal, switch to the dist/packages/php-wasm/node/ directory and link that package as well:

cd dist/packages/php-wasm/node/ 
npm link

In this project:

Run:

npm link @php-wasm/node @wp-playground/blueprints
npx nx build wp-now
node dist/packages/wp-now/cli.js start

Navigate to http://localhost:8881/

Set up auto refresh to refresh every few seconds and leave it running indefinitely.

You can use the this extenstion to do that in chrome: https://chromewebstore.google.com/detail/amazing-auto-refresh/lgjmjfjpldlhbaeinfjbgokoakpjglbn

dmsnell

this is looking good.

there's use of the /** JSDoc comment opener, but we aren't particularly following JSDoc conventions. it would be nice if we were consistent here.
do you think this same issue is likely present in the web version of the Playground? maybe it's rarer since people won't keep it open as long as they keep wp-now running? is there an easy way you can think of to refresh the PHP instance from within the Playground itself so that tools like wp-now don't also need to apply this kind of fix?

packages/nx-extensions/src/executors/built-script/schema.json

packages/wp-now/src/config.ts

dmsnell · 2023-09-14T23:26:23Z

packages/wp-now/src/pool.ts

+		maxRequests = 2000,
+		maxJobs = 5,
+	} = {}) {
+		Object.assign(this, { spawner, maxRequests, maxJobs });


this is a style note, but I think we would generally prefer to set these like this.spawner = spawner

did you see this pattern elsewhere here?

That's just something I use as shorthand for multiple assignments.

@dmsnell This has been refactored:

playground-tools/packages/wp-now/src/pool.ts

Lines 160 to 166 in 75bd3eb

Object.defineProperties(this, {

maxRequests: { value: maxRequests },

maxJobs: { value: maxJobs },

spawn: { value: spawn },

fatal: { value: fatal },

reap: { value: reap },

});

dmsnell · 2023-09-14T23:27:18Z

packages/wp-now/src/pool.ts

+	} = {}) {
+		Object.assign(this, { spawner, maxRequests, maxJobs });
+		this[Reap]();
+		this[Spawn]();


what's the purpose of using these symbols instead of simply using reap, spawn, and fatal as names?

That's just a pattern I use for private methods.

@dmsnell This has been refactored:

playground-tools/packages/wp-now/src/pool.ts

Lines 36 to 85 in 75bd3eb

const spawn = async (pool: Pool) => {

const newInstances = new Set();

if (pool.maxJobs <= 0) return newInstances;

while (pool.instanceInfo.size < pool.maxJobs) {

const info = new PoolInfo();

const instance = await pool.spawn();

pool.instanceInfo.set(instance, info);

info.active = true;

newInstances.add(instance);

}

return newInstances;

};

/**

* Reaps children if they've passed the maxRequest count.

* @param pool the pool object to work on

* @private

*/

const reap = (pool: Pool) => {

for (const [instance, info] of pool.instanceInfo) {

if (pool.maxRequests > 0 && info.requests >= pool.maxRequests) {

info.active = false;

pool.instanceInfo.delete(instance);

pool.reap(instance);

continue;

}

}

};

/**

* Handle fatal errors gracefully.

* @param pool the pool object to work on

* @param instance the php instance to clean up

* @param error the actual error that got us here

* @private

*/

const fatal = (pool: Pool, instance: PhpInstance, error: Error) => {

console.error(error);

if (instance && pool.instanceInfo.has(instance)) {

const info = pool.instanceInfo.get(instance);

info.active = false;

pool.instanceInfo.delete(instance);

}

return pool.fatal(instance, error);

};

dmsnell · 2023-09-14T23:29:06Z

packages/wp-now/src/pool.ts

+				const idleInstanceNext = this.getIdleInstance();
+
+				const next = this.backlog.shift();
+				const info = this.instances.get(idleInstanceNext);


couldn't ideInstanceNext here be false too?

While this function is kicked off by the resolution of a previous promise that SHOULD free up the instance its using, I could see how it would be possible for a FALSE to come in here if we get a request at JUST the wrong millisecond.

This is a little complicated because we're actually managing TWO pools here, one of backlogged requests and one of idle/running PHP instances, but I think I can come up with something robust to that case.

seanmorris · 2023-09-15T19:06:57Z

packages/wp-now/src/pool.ts

+					// instance is done spawning.
+					request = next(await nextInstance);
+				}
+				catch(error) {


@dmsnell Let me know what you think of this logic. The alternative to re-queuing after a failed spawn is to return a 502 bad gateway.

Interested to know your thoughts.

I'm a bit confused on what happens in these cases and probably need to think about it more. One thing that worries me is that if there is something intrinsic in the request that led to the failure, and we unshift it to the beginning of the queue, could we get into situations that lead to infinite and infinitely-fast re-death of newly spawned processes. I'm not sure if that worry is sound though, as I don't fully understand what's happening with these workers. It's not like they are OS processor or have the same characteristics of overhead and startup time.

In general, a 500 seems like an acceptable response. Why specifically a 502? The lines here are blurry.

If I recall correctly an HTTP daemon will serve a 502 if it can't properly instantiate PHP to handle a request, but a 500 seems valid as well.

@dmsnell @adamziel Lets revisit this, I don't want to forget about it.

@adamziel @dmsnell Refactored to return the error to calling code:

playground-tools/packages/wp-now/src/pool.ts

Lines 267 to 273 in 75bd3eb

// Grab the reject handler from the notfier

// promise and run it if the request rejects.

request.catch((error) => {

const resolver = this.resolvers.get(next);

this.resolvers.delete(next);

resolver.reject(fatal(this, nextInstance, error));

});

packages/wp-now/src/wp-now.ts

seanmorris · 2023-09-19T15:28:13Z

this is looking good.

there's use of the /** JSDoc comment opener, but we aren't particularly following JSDoc conventions. it would be nice if we were consistent here.

do you think this same issue is likely present in the web version of the Playground? maybe it's rarer since people won't keep it open as long as they keep wp-now running? is there an easy way you can think of to refresh the PHP instance from within the Playground itself so that tools like wp-now don't also need to apply this kind of fix?

The comment style is a force of habit, I can look over the existing style and make it consistent.

If this code need to be upstreamed into wordpress-playground, that is 100% possible. The Pool class is not coupled to anything in particular, so we can absolutely provide it out of the box in wordpress-playground.

adamziel · 2023-09-29T12:18:10Z

Is this ready for another review now?

adamziel · 2023-12-14T00:20:08Z

I saw this came up in the review queue, but there are some failing CI checks and open conversations so it seems like it's not ready for a review yet.

seanmorris · 2023-12-15T05:53:16Z

I saw this came up in the review queue, but there are some failing CI checks and open conversations so it seems like it's not ready for a review yet.

@adamziel That's going to fail until we plan a new release of wordpress-playground. The relevant code is already merged into trunk:

https://github.com/WordPress/wordpress-playground/blob/7ec4b9a0bc014baf69c3f53f1f59b40816a3aed0/packages/php-wasm/universal/src/lib/base-php.ts#L681-L683

https://github.com/WordPress/wordpress-playground/blob/7ec4b9a0bc014baf69c3f53f1f59b40816a3aed0/packages/php-wasm/universal/src/lib/php-browser.ts#L115-L138

adamziel · 2023-12-21T12:06:14Z

packages/wp-now/src/wp-now.ts

 	const nodePHPOptions: PHPLoaderOptions = {
 		requestHandler: {
 			documentRoot,
 			absoluteUrl: options.absoluteUrl,
-			isStaticFilePath: (path) => {


Removing this triggers a lint error above – seemsLikeAPHPFile is now dead code.

Strange, I was getting linter errors on my end as if it were deprecated. That's what prompted me to remove it to begin with.

I'll double check that.

@adamziel Looks like that type doesn't allow for the isStaticFilePath key. Is the isStaticFilePath key not expected to be removed? If I'm missing something, lets revisit this one tomorrow morning.

GitHub search for isStaticFilePath: https://github.com/search?q=org%3AWordPress+isStaticFilePath&type=code

Current Commit (30a561d97bdd7dee6674528dfff95c912bc734d3) https://github.com/WordPress/wordpress-playground/blob/30a561d97bdd7dee6674528dfff95c912bc734d3/packages/php-wasm/universal/src/lib/php-request-handler.ts#L17-L27

Trunk: https://github.com/WordPress/wordpress-playground/blob/trunk/packages/php-wasm/universal/src/lib/php-request-handler.ts#L17-L27

It is expected to be removed, you'll just need to remove this now-unused function, too:

@adamziel That's been removed.

packages/wp-now/src/tests/wp-now.spec.ts

adamziel · 2023-12-22T13:28:52Z

packages/wp-now/src/pool.ts

+ */
+class PoolInfo {
+	id: number; // Unique ID for debugging purposes.
+	started: number; // Time spawned.


This seems to be unused – let's remove it.

dmsnell · 2023-12-22T23:26:42Z

Looking through this I didn't see any kind of manual memory allocation for spawning a new process, but I didn't check behind the diff to see if that's there or if the question even makes sense.

Still, I was thinking about the comment about the linear memory and fragmentation (thanks for adding that) and wondered if we have any control over this, and whether we could add in a later PR a custom allocator for each PHP process so we could ensure reuse and a memory limit. E.g. if we set PHP's max memory, we could preallocate a multiple of that for every worker we build, then let new workers use the now-reaped memory from the previous worker.

dmsnell

thanks for your continued efforts on this @seanmorris. I thought it was closer to being ready, but in my review here I uncovered some things that I think do warrant some extra attention:

an architectural question about the approach we're taking for eagerly pre-spawning instances and using new instances vs. reusing existing ones.
several questions about names that don't fully match function or intent, and lots of documentation that's going 99% of the way to being useful but misses out because it's not following the JSdoc conventions.
missing types that could thread documentation and consistency through the multiple layers of the system that PHP instances and HTTP requests take through this. specifically we should have some type that describes the PhpInstance. if it's different for Node and the web that's fine, we can add a type parameter to class Pool<PhpInstance extends BaseInstance> { } and then have the NodePool and default pool extend BaseInstance however they want, but it should be there on all the functions inside Pool that work with requests. the request type is similar, and should take one of these PhpInstance values as its input.

I'm trying to summarize the issues here, but please check on all the comments as there are other items in there that would be good to address.

dmsnell · 2023-12-29T02:15:38Z

packages/wp-now/src/config.ts

@@ -43,6 +45,8 @@ export interface WPNowOptions {
 	wpContentPath?: string;
 	wordPressVersion?: string;
 	numberOfPhpInstances?: number;
+	maxRequests?: number;
+	maxJobs?: number;


what's the interplay here between maxJobs and numberOfPhpInstances?
we didn't recreate the same config value did we?

could we use numberOfPhpInstances to set maxJobs or are these somehow indicating different things: number of workers in a pool vs. number of pools? would it make sense to only use one and then to leave the existing config for WPNowOptions?

dmsnell · 2023-12-29T02:17:49Z

packages/wp-now/src/config.ts

@@ -20,6 +20,8 @@ export interface CliOptions {
 	port?: number;
 	blueprint?: string;
 	reset?: boolean;
+	maxRequests?: number;
+	maxJobs?: number;


I know it's late in the process but now as this is basically ready I'm questioning how clear these names are. we're not limiting the total number of requests, and what is a job?

what if we either renamed them or created them as sub-options like poolOptions?

something like maxRequestsPerWorkerLifespan is verbose but does far more to communicate the role of the config, and maxPhpWorkers (or even the already existing numberOfPhpInstances) skips the term for one that's already in scope when creating one of these config values.

dmsnell · 2023-12-29T02:22:51Z

packages/wp-now/src/pool.ts

+class PoolInfo {
+	id: number; // Unique ID for debugging purposes.
+	requests = 0; // Total requests processed.
+	active = false; // Whether instance is considered active.


it seems clear that active conveys some sense of being active, but I am confused on two things:

what does "active" mean? e.g. "// An instance is considered active if it's currently handling a request."

this is a property of a PoolInfo class, so which instance is considered active? we've been discussing PhpInstance and other instances. should this be "Whether this pool is [whatever active means]"?

as noted in the next comment, we would get a lot for almost no effort by moving these into JSDoc comments

dmsnell · 2023-12-29T02:23:59Z

packages/wp-now/src/pool.ts

+class PoolInfo {
+	id: number; // Unique ID for debugging purposes.
+	requests = 0; // Total requests processed.
+	active = false; // Whether instance is considered active.


since we're already explaining these, let's use JSDoc syntax so that the comments follow the values.

/** Unique ID for debugging purposes. */ id: number; /** Total requests processed by this pool/instance/something? */ requests = 0; /** This pool is considered active if X Y Z */ active = false;

dmsnell · 2023-12-29T02:31:04Z

packages/wp-now/src/pool.ts

+};
+
+/**
+ * Reaps children if they've passed the maxRequest count.


this is a description of what the code currently does, but it would be less fragile as a comment if we described why it does what it does.

something like this…

/** * Reaps (terminates) all of the PHP instances * in a given pool that are deemed "too old." * * In order to prevent memory leaks inherent in * a long-living PHP worker, this function reaps * PHP instances after they have handled more * than the configured number of maximum requests. * * There could be different conditions for reaping, * but the intent is to limit the extent of a * memory leak and ensure any proper cleanup happens * even if a process goes astray. * * @private * * @param pool Reap old PHP instances in this pool. */ const reap = (pool: Pool) => { … }

dmsnell · 2023-12-29T03:36:16Z

packages/wp-now/src/pool.ts

+
+				request = next(nextInstance);
+			} catch (error) {
+				// Grab the reject handler from the notfier


typo: notfier -> notifier

dmsnell · 2023-12-29T03:39:20Z

packages/wp-now/src/pool.ts

+			this.running.add(nextInstance);
+			info.requests++;
+
+			let request;


notice that at this point, if we don't rename the request type, we will have a type and a variable with the same name in the same scope. they occupy different namespaces in TypeScript, which means there's no clash, but that's needlessly confusing if someone is trying to figure out what's happening

dmsnell · 2023-12-29T03:42:29Z

packages/wp-now/src/pool.ts

+			try {
+				const newInstances = await spawn(this);
+
+				// ... but, if we've just spanwed a fresh


why are we preferring a newly-spawned instance over the one that just finished? is there any advantage to this inside our WASM world?

I would have guessed that passing the next request to the existing worker would exhaust the worker faster and prevent needing to create additional workers, which would lead to a lower overall memory use.

this also leads me to wonder about the pre-allocation of workers and if that's necessary. if we have maxJobs (or any more suitable name) and we're going to eagerly pay the costs of allocating them, why not defer allocation and initialization until those workers are required: that is, until we have a request come in when there are no non-active workers.

would you see any problem in reaping workers as soon as they complete a request and are over the max-requests threshold, and then only spawning new workers once there are no available non-active workers in the pool?

dmsnell · 2023-12-29T03:49:49Z

packages/wp-now/src/pool.ts

+				return;
+			}
+
+			const completed = onCompleted(nextInstance);


this line has no explanation, but it's calling onCompleted from within onCompleted, and is a bit dizzying trying to follow the flow of data inside of it, and inside the recursion.

I think here a different model that examines one request at a time could potentially remove a fair amount of logic and simplify everything in this flow. I'm curious to see if you also see the value and opportunity there.

dmsnell · 2023-12-29T03:58:05Z

packages/wp-now/src/pool.ts

@@ -0,0 +1,322 @@
+type PhpInstance = any;
+
+type request = (instance: PhpInstance) => Promise<any>;


Let's leave our types capitalized: request -> Request

additionally, it would help if we had a proper type for an instance and for a request. they don't need to be here, but somewhere would be good. here is a start I tossed together.

import { HTTPMethod } from '@php-wasm/universal'; interface HttpRequest { url: string; headers: Record<string, string>; method: HTTPMethod; files: Record<string, File>; body: string; } interface BaseInstance { request(data: HttpRequest): Promise<void>; }

it could be that these already exist somewhere and I didn't see them. they will help later on, in places like start-server.ts

adamziel · 2024-01-09T13:18:30Z

For posterity – this would be useful in php-wasm core as the web version of Playground suffers from the same memory leak issue.

@dmsnell

…a certain number of requests Supersedes WordPress/playground-tools#110 Prevents PHP's memory leaks from causing the system to stop working once memory has been exhausted. It follows in the footsteps of systems like PHP-FPM by limiting the number of requests a PHP "thread" can handle before it is discarded and restarted. Implementing a solution in core enables all downstream consumers of the PHP WASM package to benefit. ## How it works Adds a rotatedPHP function that takes a PHP factory function and returns a Proxy that manages the PHP instance internally. The Proxy will discard the PHP instance after a certain number of requests, and will create a new instance when the next request comes in. Example: ```ts const php = rotatedPHP({ createPhp: () => WebPHP.load('8.0'), maxRequests: 50, }); // After 50 request() calls, the PHP instance will be discarded and a new one will be created. // It all happens internally. The caller doesn't know anything about this. ``` I started by porting the "Process Pool" idea from WordPress/playground-tools#110, but I realized that: * The logic was complicated * We only need to manage a single PHP instance, not a pool of them * Worker thread is built to a single PHP instance and not architected to handle a pool So I decided to simplify the logic and just manage a single PHP instance. ## Remaining work * Unit tests * Comments * Better names perhaps, `rotatedPHP` doesn't seem ideal CC @dmsnell

adamziel · 2024-01-31T17:06:46Z

I'm exploring an alternative in Playground core here:

WordPress/wordpress-playground#990

…a certain number of requests (#990) Swaps the internal PHP runtime for a fresh one after a certain number of requests are handled. ## Rationale PHP and PHP extension have a memory leak. Each request leaves the memory a bit more fragmented and with a bit less available space than before. Eventually, new allocations start failing. Rotating the PHP instance may seem like a workaround, but it's actually what PHP-FPM does natively: Implementing this solution in core enables all downstream consumers of the PHP WASM package, like `wp-now`, to benefit. ## How it works Adds a `rotatePHP` function that keeps track of the `BasePHP.run()` calls and replaces the Emscripten runtime with a new one after a certain number of calls. Example: ```ts const loadRuntime = async () => { return await NodePHP.loadRuntime("8.0"); } const php = await rotatedPHP({ php: new NodePHP(await loadRuntime(), { documentRoot: '/test-root', }), recreateRuntime: loadRuntime, maxRequests: 50 }); // After 50 request() calls, the PHP runtime will be discarded and a new one will be created. // It all happens internally. The caller doesn't know anything about this. ``` I started by porting the "Process Pool" idea ([see the branch](https://github.com/WordPress/wordpress-playground/compare/harvested-memory-pool?expand=1)) from WordPress/playground-tools#110, but I realized that: * The logic was complicated * We only need to manage a single PHP instance, not a pool of them * Worker thread is built to a single PHP instance and not architected to handle a pool So I decided to simplify the logic and just manage a single PHP instance. ## Other changes * Implements `BasePHP.hotSwapPHPRuntime()` that opens the door to switching PHP versions dynamically and without a full page reload. * Makes the `BasePHP.semaphore` property public to enable conflict-free runtime swaps * Adds the `runtime.initialized` and `runtime.beforedestroy` events for OPFS handlers to re-bind on runtime swap. * Tiny unrelated change: Migrates the OPFS handler from monkey-patching the `php.run` method to relying on event listeners. The `rotatePHP` function went through a similar journey and I updated that other code path while I was on it. Supersedes WordPress/playground-tools#110

adamziel · 2024-02-02T09:48:05Z

WordPress/wordpress-playground#990 just landed and enables a drop-in solution to the memory leak problem.

Instead of this:

const php = await NodePHP.load("8.0");

You would now do this:

import { rotatePHPRuntime } from '@php-wasm/universal';

const loadRuntime = async () => await NodePHP.loadRuntime("8.0");

const php = new NodePHP(await loadRuntime(), {
	documentRoot: '/test-root',
});

await rotatePHPRuntime({
	php,
	recreateRuntime: loadRuntime,
	maxRequests: 400
});

And the PHP runtime would be magically cleaned up and recreated every 400 requests.

Adapting wp-now will require a follow-up PR. Would you be up for exploring that @sejas or @bgrgicak?

sejas · 2024-02-05T10:16:54Z

Nice!
I can create the PR for wp-now.
Instead of adding a maxRequest, it would be super cool to set it to ' auto' and the php instance could track if it needs "clean" the memory and rotate the runtime.

@adamziel , I think we need to release a new version of @php-wasm/node

sejas · 2024-02-05T10:33:42Z

I created the PR for wp-now: #152
It just needs to be updated with the new version of @php-wasm/node that contains the new function rotatePHPRuntime.

Currently is on version 0.5.6 that was released 18 days ago, and the new function was introduced 4 days ago on WordPress/wordpress-playground#990

https://www.npmjs.com/package/@php-wasm/node

adamziel · 2024-02-05T11:35:17Z

Instead of adding a maxRequest, it would be super cool to set it to ' auto' and the php instance could track if it needs "clean" the memory and rotate the runtime.

Agreed, I started a new issue to track it at WordPress/wordpress-playground#998

@adamziel , I think we need to release a new version of @php-wasm/node

You're right! v0.6.0 release is in progress now

…xport-functionality Add import and export logic

seanmorris added 10 commits September 13, 2023 22:01

Add pooling and request limiting to prevent memory leaks

6c12e78

Adding Pool class.

e968403

Reverting change.

b515f13

Removing console.log

5c7b52d

Max request options.

a33b0c5

Tweaks.

f8326cd

Tweaks.

cb383fe

Initialize all instances

ac4ea1b

Tweak.

0b9044f

Tweak.

fc0f995

dmsnell reviewed Sep 14, 2023

View reviewed changes

seanmorris added 6 commits September 14, 2023 20:02

Consolidating FINALLY block.

e18b1fd

Cleaner pooling algorithm.

0ffaad7

Comments

3d26067

Comments.

909d08a

Comments.

1c91f34

Catch spawn failures

a86c4c9

seanmorris commented Sep 15, 2023

View reviewed changes

seanmorris added 3 commits September 15, 2023 15:50

Making private methods privater.

4b634bb

Tweaks.

3f7b796

Tweaks.

db3c5d6

seanmorris commented Sep 15, 2023

View reviewed changes

packages/wp-now/src/wp-now.ts Outdated Show resolved Hide resolved

seanmorris added 4 commits September 15, 2023 21:34

Correcting CLI switches, killing instances when done.

68f6290

Detect error in test.

9167e77

Correcting multi-await

70c3a22

Correcting multi-await

aebc3d5

Scaling back default maxRequests

96a2e82

adamziel mentioned this pull request Sep 29, 2023

wp-now: out of memory error when running for many hours #96

Open

adamziel mentioned this pull request Dec 19, 2023

Fix a cross-device mv error by using a tmp directory inside document root WordPress/wordpress-playground#886

Merged

seanmorris added 3 commits December 21, 2023 03:07

Incrementing verion numbers

c3b2269

Formatting.

1f9f696

Lint & Tests.

f352bb8

adamziel reviewed Dec 21, 2023

View reviewed changes

packages/wp-now/src/tests/wp-now.spec.ts Show resolved Hide resolved

seanmorris added Bug Something isn't working wp-now labels Dec 22, 2023

adamziel reviewed Dec 22, 2023

View reviewed changes

seanmorris added 5 commits December 27, 2023 05:18

Merge conflict.

0e6c8ba

PR comment tweaks.

0ad0b9d

Restoring codemirror dependency

5d18b81

Revering extra change

b09b13b

PR comment tweaks.

0bd1222

dmsnell suggested changes Dec 29, 2023

View reviewed changes

adamziel unassigned seanmorris Jan 9, 2024

adamziel mentioned this pull request Jan 31, 2024

Memory leak: add rotatedPHP to kill and recreate PHP instances after a certain number of requests WordPress/wordpress-playground#990

Merged

adamziel closed this Feb 2, 2024

johnhooks pushed a commit to johnhooks/playground-tools that referenced this pull request Oct 11, 2024

Merge pull request WordPress#110 from artemiomorales/try/add-import-e…

6ef8e8d

…xport-functionality Add import and export logic

	Object.defineProperties(this, {
	maxRequests: { value: maxRequests },
	maxJobs: { value: maxJobs },
	spawn: { value: spawn },
	fatal: { value: fatal },
	reap: { value: reap },
	});

	const spawn = async (pool: Pool) => {
	const newInstances = new Set();

	if (pool.maxJobs <= 0) return newInstances;

	while (pool.instanceInfo.size < pool.maxJobs) {
	const info = new PoolInfo();
	const instance = await pool.spawn();
	pool.instanceInfo.set(instance, info);
	info.active = true;
	newInstances.add(instance);
	}

	return newInstances;
	};

	/**
	* Reaps children if they've passed the maxRequest count.
	* @param pool the pool object to work on
	* @private
	*/
	const reap = (pool: Pool) => {
	for (const [instance, info] of pool.instanceInfo) {
	if (pool.maxRequests > 0 && info.requests >= pool.maxRequests) {
	info.active = false;
	pool.instanceInfo.delete(instance);
	pool.reap(instance);
	continue;
	}
	}
	};

	/**
	* Handle fatal errors gracefully.
	* @param pool the pool object to work on
	* @param instance the php instance to clean up
	* @param error the actual error that got us here
	* @private
	*/
	const fatal = (pool: Pool, instance: PhpInstance, error: Error) => {
	console.error(error);

	if (instance && pool.instanceInfo.has(instance)) {
	const info = pool.instanceInfo.get(instance);
	info.active = false;
	pool.instanceInfo.delete(instance);
	}

	return pool.fatal(instance, error);
	};

	// Grab the reject handler from the notfier
	// promise and run it if the request rejects.
	request.catch((error) => {
	const resolver = this.resolvers.get(next);
	this.resolvers.delete(next);
	resolver.reject(fatal(this, nextInstance, error));
	});

		@@ -0,0 +1,322 @@
		type PhpInstance = any;

		type request = (instance: PhpInstance) => Promise<any>;

Add pooling and request limiting to prevent memory leaks #110

Add pooling and request limiting to prevent memory leaks #110

Conversation

seanmorris commented Sep 14, 2023 • edited Loading

What is this PR doing?

What problem is it solving?

How is the problem addressed?

Testing Instructions

in wordpress-playground

In this project:

dmsnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmorris Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmorris Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmorris Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmorris Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

seanmorris commented Sep 19, 2023

adamziel commented Sep 29, 2023 • edited Loading

adamziel commented Dec 14, 2023

seanmorris commented Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell commented Dec 22, 2023

dmsnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Jan 9, 2024

adamziel commented Jan 31, 2024 • edited Loading

adamziel commented Feb 2, 2024 • edited Loading

sejas commented Feb 5, 2024

sejas commented Feb 5, 2024 • edited Loading

adamziel commented Feb 5, 2024

seanmorris commented Sep 14, 2023 •

edited

Loading

in `wordpress-playground`

seanmorris Nov 27, 2023 •

edited

Loading

seanmorris Nov 27, 2023 •

edited

Loading

seanmorris Sep 15, 2023 •

edited

Loading

seanmorris Nov 27, 2023 •

edited

Loading

adamziel commented Sep 29, 2023 •

edited

Loading

seanmorris commented Dec 15, 2023 •

edited

Loading

adamziel commented Jan 31, 2024 •

edited

Loading

adamziel commented Feb 2, 2024 •

edited

Loading

sejas commented Feb 5, 2024 •

edited

Loading