Skip to content

fix(pg-cloudflare): emit an error when a write without a callback fails - #3859

Open
dax-side wants to merge 1 commit into
brianc:masterfrom
dax-side:fix-3757-cloudflare-write-error
Open

dax-side wants to merge 1 commit into
brianc:masterfrom
dax-side:fix-3757-cloudflare-write-error

Conversation

@dax-side

Copy link
Copy Markdown

Part of #3757 (the error-reporting half only).

Connection._send() calls stream.write(buffer) with no callback. In CloudflareSocket.write() the callback defaulted to a no-op, so when the Cloudflare writer rejected, the rejection was dropped. The app then only saw a later "Connection terminated unexpectedly", with no hint of why. charmander asked on the issue whether a useful write() rejection was being swallowed, and it is.

With this change, if no callback was passed, the rejection is emitted as 'error', the same way a net.Socket reports a failed write. Connection already listens for 'error' on the stream and passes it on to the client. Callers that pass a callback (such as end()) still get the error in the callback, and nothing else changes.

Two tests added to pg-esm-test/pg-cloudflare.test.js, in the style of the existing ones (fake _cfWriter):

  • a write with no callback whose writer rejects emits 'error' with the original error
  • a write with a callback whose writer rejects gets the error in the callback, and no 'error' is emitted

This is the same approach as #3784, which its author closed on Oct 23 without a review.

This does not change how large writes are sent (the chunking/backpressure part of the issue). I couldn't verify that part without deploying a Worker.

pg's Connection calls stream.write(buffer) without a callback. When the
Cloudflare writer rejected, the error went to a default no-op callback and
was lost; the application only saw a later "Connection terminated
unexpectedly".

Now, when no callback is given, the rejection is emitted as an 'error'
event, as a Node net.Socket would do. A caller that passes a callback still
gets the error there, as before.

Part of brianc#3757 (the error-reporting half; chunking large writes is not
changed here).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant