Repository navigation
fix(pg-cloudflare): do not emit an error for the read that end() cancels - #3806
WolfieLeader wants to merge 2 commits into
Conversation
Since brianc#3747, end() closes the Cloudflare socket while _listen() still waits on a read. workerd rejects that read with "This socket has been closed.", and pg re-emits it as a client 'error' after end() resolved. With no 'error' listener this is an unhandled rejection. Ignore a rejected read once end() has been called. Other read failures still emit 'error'.
robingenz
left a comment
There was a problem hiding this comment.
Tested this change, and it works for me.
I built packages/pg-cloudflare from this branch (6f83843) and ran it against PostgreSQL 17 with pg 8.23.1:
wrangler dev(wrangler 4.142.0, workerd 1.20260926.1):client.end(),pool.end()and a pool's idle timeout no longer emitThis socket has been closed.Without the change, all three do.- Workers Vitest pool (
@cloudflare/vitest-pool-workers0.22.0, workerd 1.20260815.1): same result. There the old error text isStream was cancelled. - A real failure is still reported.
pg_terminate_backend()duringSELECT pg_sleep(5)fails the query withterminating connection due to administrator command, and the client emitsConnection terminated unexpectedly.
One suggestion: the new test covers the read that end() cancels, but nothing checks that a read failing before end() still fails _listen(). A small test would guard the throw e branch:
it('should still fail listening when a read fails before end()', async () => {
const socket = new CloudflareSocket()
socket._cfReader = { read: () => Promise.reject(new Error('Network connection lost.')) }
await assert.rejects(socket._listen(), /Network connection lost/)
})With it, node --test --conditions=workerd pg-cloudflare.test.js in packages/pg-esm-test passes 6 of 6 on this branch. On master, your new test fails with TypeError: This socket has been closed., as expected.
The two cases from the latest comment on #3805, a read rejected after the peer already closed the socket and an emit('error') with no listener left, are not covered here. Worth a follow-up in this PR or a second one.
|
Wow, thank you @robingenz Ill check that |
|
Thanks @robingenz for testing this on That commit also covers the first case from @SlambertDK's comment: |
Fixes #3805.
Since #3747,
CloudflareSocket.end()closes the socket while_listen()still waits on a read. workerd rejects that read with "This socket has been closed.", and pg re-emits it as a client'error'afterend()resolved. With no'error'listener, that is an unhandled rejection.This change ignores a rejected read once
end()has been called. Other read failures still emit'error'.Tests:
packages/pg-esm-test/pg-cloudflare.test.js. A fake reader keeps the read pending until the fakeclose()rejects it, as workerd does. Before: 36 pass, 1 fail. After: 37 pass.await client.end()after a query:pg_terminate_backend) still reaches the client as'error'.Not changed:
_listenOnce(), the SSL handshake read. In workerd,end()during the handshake did not reject that read.