-
Notifications
You must be signed in to change notification settings - Fork 42
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
Add pooling and request limiting to prevent memory leaks #110
Changes from all commits
6c12e78
e968403
b515f13
5c7b52d
a33b0c5
f8326cd
cb383fe
ac4ea1b
0b9044f
fc0f995
e18b1fd
0ffaad7
3d26067
909d08a
1c91f34
a86c4c9
4b634bb
3f7b796
db3c5d6
68f6290
9167e77
70c3a22
aebc3d5
96a2e82
0cb98b7
a73874f
326b410
966a9df
488ad3c
a7708af
e01d6d4
eca5995
3d8a1c2
271c935
a7581dc
8d968d7
04a3e5e
65c28e2
a1b809e
102adbc
954d33f
6bc105d
b3c6a08
be0c0cb
fabcafa
56484b5
c59bea8
c133a1e
a657040
75bd3eb
c3b2269
1f9f696
f352bb8
0e6c8ba
0ad0b9d
5d18b81
b09b13b
0bd1222
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,28 @@ | ||
{ | ||
"landingPage": "/wp-admin/post.php?post=4&action=edit", | ||
"steps": [ | ||
{ | ||
"step": "login", | ||
"username": "admin", | ||
"password": "password" | ||
}, | ||
{ | ||
"step": "installTheme", | ||
"themeZipFile": { | ||
"resource": "wordpress.org/themes", | ||
"slug": "adventurer" | ||
} | ||
}, | ||
{ | ||
"step": "installPlugin", | ||
"pluginZipFile": { | ||
"resource": "wordpress.org/plugins", | ||
"slug": "interactive-code-block" | ||
} | ||
}, | ||
{ | ||
"step": "runPHP", | ||
"code": "<?php require '/wordpress/wp-load.php'; wp_insert_post(['post_title' => 'Interactive code block demo!','post_content' => '<!-- wp:wordpress-playground/interactive-code {\"code\":\"PD9waHAKCmVjaG8gIlRoaXMgY29kZSBzbmlwcGV0IGlzIGludGVyYWN0aXZlISI7Cg==\",\"cachedOutput\":\"VGhpcyBjb2RlIHNuaXBwZXQgaXMgaW50ZXJhY3RpdmUh\"} --><div class=\"wp-block-wordpress-playground-interactive-code\">PD9waHAKCmVjaG8gIlRoaXMgY29kZSBzbmlwcGV0IGlzIGludGVyYWN0aXZlISI7Cg==</div><!-- /wp:wordpress-playground/interactive-code -->', 'post_status' => 'publish', 'post_type' => 'post',]);" | ||
} | ||
] | ||
"landingPage": "/wp-admin/post.php?post=4&action=edit", | ||
"steps": [ | ||
{ | ||
"step": "login", | ||
"username": "admin", | ||
"password": "password" | ||
}, | ||
{ | ||
"step": "installTheme", | ||
"themeZipFile": { | ||
"resource": "wordpress.org/themes", | ||
"slug": "adventurer" | ||
} | ||
}, | ||
{ | ||
"step": "installPlugin", | ||
"pluginZipFile": { | ||
"resource": "wordpress.org/plugins", | ||
"slug": "interactive-code-block" | ||
} | ||
}, | ||
{ | ||
"step": "runPHP", | ||
"code": "<?php require '/wordpress/wp-load.php'; wp_insert_post(['post_title' => 'Interactive code block demo!','post_content' => '<!-- wp:wordpress-playground/interactive-code {\"code\":\"PD9waHAKCmVjaG8gIlRoaXMgY29kZSBzbmlwcGV0IGlzIGludGVyYWN0aXZlISI7Cg==\",\"cachedOutput\":\"VGhpcyBjb2RlIHNuaXBwZXQgaXMgaW50ZXJhY3RpdmUh\"} --><div class=\"wp-block-wordpress-playground-interactive-code\">PD9waHAKCmVjaG8gIlRoaXMgY29kZSBzbmlwcGV0IGlzIGludGVyYWN0aXZlISI7Cg==</div><!-- /wp:wordpress-playground/interactive-code -->', 'post_status' => 'publish', 'post_type' => 'post',]);" | ||
} | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,8 @@ export interface CliOptions { | |
port?: number; | ||
blueprint?: string; | ||
reset?: boolean; | ||
maxRequests?: number; | ||
maxJobs?: number; | ||
} | ||
|
||
export const enum WPNowMode { | ||
|
@@ -43,6 +45,8 @@ export interface WPNowOptions { | |
wpContentPath?: string; | ||
wordPressVersion?: string; | ||
numberOfPhpInstances?: number; | ||
maxRequests?: number; | ||
maxJobs?: number; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the interplay here between could we use |
||
blueprintObject?: Blueprint; | ||
reset?: boolean; | ||
} | ||
|
@@ -54,6 +58,8 @@ export const DEFAULT_OPTIONS: WPNowOptions = { | |
projectPath: process.cwd(), | ||
mode: WPNowMode.AUTO, | ||
numberOfPhpInstances: 1, | ||
maxRequests: 128, | ||
maxJobs: 1, | ||
reset: false, | ||
}; | ||
|
||
|
@@ -111,6 +117,8 @@ export default async function getWpNowConfig( | |
phpVersion: args.php as SupportedPHPVersion, | ||
projectPath: args.path as string, | ||
wordPressVersion: args.wp as string, | ||
maxRequests: args.maxRequests as number, | ||
adamziel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
maxJobs: args.maxJobs as number, | ||
port, | ||
reset: args.reset as boolean, | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import { LatestSupportedPHPVersion, PHPResponse } from '@php-wasm/universal'; | ||
import { NodePHP } from '@php-wasm/node'; | ||
import { Pool } from './pool'; | ||
|
||
export type NodePoolOptions = { | ||
spawn?: () => Promise<NodePHP>; | ||
reap?: (instance: NodePHP) => void; | ||
fatal?: (instance: NodePHP, error: any) => any; | ||
maxRequests?: number; | ||
maxJobs?: number; | ||
}; | ||
|
||
const defaultSpawn = async () => await NodePHP.load(LatestSupportedPHPVersion); | ||
|
||
const defaultFatal = (instance: NodePHP, error: any) => | ||
new PHPResponse( | ||
500, | ||
{}, | ||
new TextEncoder().encode( | ||
`500 Internal Server Error:\n\n${String( | ||
error && error.stack ? error.stack : error | ||
)}` | ||
) | ||
); | ||
|
||
const defaultReap = (instance: NodePHP) => { | ||
try { | ||
instance.exit(); | ||
} catch { | ||
// Do nothing. | ||
} | ||
}; | ||
|
||
export class NodePool extends Pool { | ||
constructor({ | ||
maxRequests = 128, | ||
maxJobs = 1, | ||
spawn = defaultSpawn, | ||
fatal = defaultFatal, | ||
reap = defaultReap, | ||
} = {}) { | ||
super({ maxRequests, maxJobs, spawn, fatal, reap }); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, andmaxPhpWorkers
(or even the already existingnumberOfPhpInstances
) skips the term for one that's already in scope when creating one of these config values.