Commit 4c5aa92

committed

PHP: Fix deadlock in poll(2)

We patch poll(2) with a JavaScript implementation. This PR resolves two problems in it: ### Infinite wait on poll(socketd, POLLIN) Polling a socket for more data with `poll(socketd, POLLIN)` used to hang once the data was exhausted and the socket closed because we never notified the caller about POLLHUP. We even have a related workaround in wordpress-components: ```php $bytes = fread( $this->file_pointer, $n ); /** * Workaround for a streaming bug in WordPress Playground. * * Without the feof() call, Playground doesn't notice when the stream reaches EOF. * The feof() call in internal_reached_end_of_data() somehow does not trigger the * EOF event. It must be here, right after fread(). * * @todo: Improve the streaming support in WordPress Playground. */ feof( $this->file_pointer ); if ( false === $bytes ) { throw new ByteStreamException( 'Failed to read from file' ); } ``` This PR makes poll(socketd, POLLIN) aware of a POLLHUP event. I'm not sure if that's POSIX-correct and we should verify that before merging, but it actually solves the issue. CC @brandonpayton ### Not recognizing a socketd as socket PHP.wasm supports child processes and keeps track of them in PHPWASM.child_proc_by_fd. However, we never clean up the socketd => procInfo mapping. When a socketd number is getting reused, PHP would mistakenly identify it as a child process and never poll the actual socket for data. This PR works around it by testing for socket-ness first and for child-process-ness second. This is ultimatly a hack. In a follow up PR we should find a way to actually remove closed socketd from the child process map. ## Remaining work * Test coverage * Start an issue to actually cleanup child process ids

1 parent fb0ffb5 commit 4c5aa92Copy full SHA for 4c5aa92

File tree

23 files changed

+8436

-9437

lines changed

packages/php-wasm
- compile/php
  - php_wasm.c
  - phpwasm-emscripten-library.js
- node
  - asyncify
  - jspi
  - src/test
    - php-networking.spec.ts
    - php.spec.ts
- web/public/php/jspi
  - 8_3_0
    - php_8_3.wasm
  - php_8_3.js

23 files changed

+8436

-9437

lines changed

`‎packages/php-wasm/compile/php/php_wasm.c‎`

Lines changed: 37 additions & 25 deletions

Original file line number	Diff line number	Diff line change
`@@ -147,15 +147,12 @@ EM_JS(int, wasm_poll_socket, (php_socket_t socketd, int events, int timeout), {`
`147`	`147`
`148`	`148`	`return returnCallback((wakeUp) => {`
`149`	`149`	`const polls = [];`
`150`		`- if (socketd in PHPWASM.child_proc_by_fd) {`
`151`		`- // This is a child process-related socket.`
`152`		`- const procInfo = PHPWASM.child_proc_by_fd[socketd];`
`153`		`- if (procInfo.exited) {`
`154`		`- wakeUp(0);`
`155`		`- return;`
`156`		`- }`
`157`		`- polls.push(PHPWASM.awaitEvent(procInfo.stdout, 'data'));`
`158`		`- } else if (FS.isSocket(FS.getStream(socketd)?.node.mode)) {`
	`150`	`+ /**`
	`151`	`+ * Check for socket-ness first. We don't clean up child_proc_by_fd yet and`
	`152`	`+ * sometimes get duplicate entries. isSocket is more reliable out of the two –`
	`153`	`+ * let's check for it first.`
	`154`	`+ */`
	`155`	`+ if (FS.isSocket(FS.getStream(socketd)?.node.mode)) {`
`159`	`156`	`// This is, most likely, a websocket. Let's make sure.`
`160`	`157`	`const sock = getSocketFromFD(socketd);`
`161`	`158`	`if (!sock) {`
`@@ -184,23 +181,38 @@ EM_JS(int, wasm_poll_socket, (php_socket_t socketd, int events, int timeout), {`
`184`	`181`	`return;`
`185`	`182`	`}`
`186`	`183`	`for (const ws of webSockets) {`
`187`		`- if (events & POLLIN \|\| events & POLLPRI) {`
`188`		`- polls.push(PHPWASM.awaitData(ws));`
`189`		`- lookingFor.add('POLLIN');`
`190`		`- }`
`191`		`- if (events & POLLOUT) {`
`192`		`- polls.push(PHPWASM.awaitConnection(ws));`
`193`		`- lookingFor.add('POLLOUT');`
`194`		`- }`
`195`		`- if (events & POLLHUP) {`
`196`		`- polls.push(PHPWASM.awaitClose(ws));`
`197`		`- lookingFor.add('POLLHUP');`
`198`		`- }`
`199`		`- if (events & POLLERR \|\| events & POLLNVAL) {`
`200`		`- polls.push(PHPWASM.awaitError(ws));`
`201`		`- lookingFor.add('POLLERR');`
`202`		`- }`
	`184`	`+ if (events & POLLIN \|\| events & POLLPRI) {`
	`185`	`+ polls.push(PHPWASM.awaitData(ws));`
	`186`	`+ lookingFor.add('POLLIN');`
	`187`	`+ }`
	`188`	`+ if (events & POLLOUT) {`
	`189`	`+ polls.push(PHPWASM.awaitConnection(ws));`
	`190`	`+ lookingFor.add('POLLOUT');`
	`191`	`+ }`
	`192`	`+ // Notify the user the socket is now closed even if the only requested`
	`193`	`+ // in or out events.`
	`194`	`+ if (`
	`195`	`+ events & POLLHUP \|\|`
	`196`	`+ events & POLLIN \|\|`
	`197`	`+ events & POLLOUT \|\|`
	`198`	`+ events & POLLERR`
	`199`	`+ ) {`
	`200`	`+ polls.push(PHPWASM.awaitClose(ws));`
	`201`	`+ lookingFor.add('POLLHUP');`
	`202`	`+ }`
	`203`	`+ if (events & POLLERR \|\| events & POLLNVAL) {`
	`204`	`+ polls.push(PHPWASM.awaitError(ws));`
	`205`	`+ lookingFor.add('POLLERR');`
	`206`	`+ }`
`203`	`207`	`}`
	`208`	`+ } else if (socketd in PHPWASM.child_proc_by_fd) {`
	`209`	`+ // This is a child process-related socket.`
	`210`	`+ const procInfo = PHPWASM.child_proc_by_fd[socketd];`
	`211`	`+ if (procInfo.exited) {`
	`212`	`+ wakeUp(0);`
	`213`	`+ return;`
	`214`	`+ }`
	`215`	`+ polls.push(PHPWASM.awaitEvent(procInfo.stdout, 'data'));`
`204`	`216`	`} else {`
`205`	`217`	`setTimeout(function () {`
`206`	`218`	`wakeUp(1);`

`‎packages/php-wasm/compile/php/phpwasm-emscripten-library.js‎`

Lines changed: 24 additions & 1 deletion

Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,15 @@ const LibraryExample = {`
`66`	`66`	`}`
`67`	`67`	`}`
`68`	`68`	`}`
`69`		`- };`
	`69`	`+ };`
	`70`	`+`
	`71`	`+ // Clean up the fd -> childProcess mapping when the fd is closed:`
	`72`	`+ const originalClose = FS.close;`
	`73`	`+ FS.close = function (stream) {`
	`74`	`+ originalClose(stream);`
	`75`	`+ delete PHPWASM.child_proc_by_fd[stream.fd];`
	`76`	`+ };`
	`77`	`+`
`70`	`78`	`PHPWASM.child_proc_by_fd = {};`
`71`	`79`	`PHPWASM.child_proc_by_pid = {};`
`72`	`80`	`PHPWASM.input_devices = {};`
`@@ -440,6 +448,21 @@ const LibraryExample = {`
`440`	`448`	`PHPWASM.child_proc_by_pid[ProcInfo.pid] = ProcInfo;`
`441`	`449`
`442`	`450`	`cp.on('exit', function (code) {`
	`451`	`+ for (const fd of [`
	`452`	`+ // The child process exited. Let's clean up its output streams:`
	`453`	`+ ProcInfo.stdoutChildFd,`
	`454`	`+ ProcInfo.stderrChildFd,`
	`455`	`+ // Note we're not closing stdinFd as the parent still might be holding on to it.`
	`456`	`+`
	`457`	`+ // We won't close these because the parent process is responsible for that:`
	`458`	`+ // ProcInfo.stdoutParentFd,`
	`459`	`+ // ProcInfo.stderrParentFd,`
	`460`	`+ ]) {`
	`461`	`+ if(FS.streams[fd] && !FS.isClosed(FS.streams[fd])) {`
	`462`	`+ FS.close(FS.streams[fd]);`
	`463`	`+ }`
	`464`	`+ }`
	`465`	`+`
`443`	`466`	`ProcInfo.exitCode = code;`
`444`	`467`	`ProcInfo.exited = true;`
`445`	`468`	`// Emit events for the wasm_poll_socket function.`