porsager / porsager/postgres

Cloudflare polyfill: unhandled rejection when a connection closes (read() emits 'error' after listeners are removed)

Open
#1,196 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
8.7k
Forks
374
Avg merge
11d 16h
Merged PRs (30d)
1

Description

Version

postgres@3.4.9, running on Cloudflare Workers (cf/polyfills.js) behind Hyperdrive.

Summary

Closing a connection intermittently produces an unhandled promise rejection. The rejection escapes from inside read()'s own catch block, so there is no way for calling code to handle it. On Workers this surfaces as an unhandledRejection logged against whatever request happened to be in flight:

⨯ unhandledRejection:     at async read (worker.js:...)

It is cosmetic — the request still succeeds — but it is unactionable log noise attributed to innocent requests, and it fires on essentially every connection teardown.

Cause

Three pieces in cf/polyfills.js combine:

  1. read() is invoked fire-and-forget with no .catch() (line 163):

    tcp.ssl ? readFirst() : read()
    
  2. read()'s error path calls error() (lines 197–206), which emits 'error' on the EventEmitter (lines 213–216):

    async function read() {
      try {
        let done, value
        while (({ done, value } = await tcp.reader.read(), !done))
          tcp.emit('data', Buffer.from(value))
      } catch (err) {
        error(err)          // <-- runs inside the catch
      }
    }
    
    function error(err) {
      tcp.emit('error', err)   // <-- throws if no 'error' listener
      tcp.emit('close')
    }
    
  3. src/connection.js removes every listener on close (line 447, inside closed()):

    socket.removeAllListeners()
    

Closing the socket starts two things concurrently:

  • tcp.raw.closed resolves → close()emit('close')connection.js closed()removeAllListeners()
  • the pending tcp.reader.read() rejects → catcherror(err)emit('error', err)

If the listener removal wins that race, emit('error') has no listeners. Node's EventEmitter contract is to throw in that case, and because the throw happens inside read()'s catch, nothing catches it — read()'s promise rejects with no handler attached.

This is timing-dependent, which matches the intermittent behaviour we see.

Reproduction

Any Workers deployment that opens a client per request and calls sql.end() when the request finishes will hit it within a handful of requests. It reproduces with both end() and end({ timeout: 1 }), i.e. graceful and forced teardown alike, since both end with tcp.raw.close() while a read is pending.

Suggested fix

Either would resolve it:

  • Make error() tolerate a listener-less socket, e.g. tcp.listenerCount('error') && tcp.emit('error', err) before emitting 'close'.
  • Attach a rejection handler where read() is started: tcp.ssl ? readFirst() : read().catch(() => {}).

Note readFirst() (lines 208–211) has no try/catch at all, so it can reject directly for the SSL path — worth covering in the same change.

Happy to open a PR if a preferred direction is indicated.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in cf/polyfills.js at the fire-and-forget read() and readFirst() calls, then trace the error() path alongside src/connection.js closed(), which removes all listeners. Reproduce teardown with end() and end({ timeout: 1 }) in a Cloudflare Workers deployment; done means connection closing no longer produces an unhandled rejection on the read path.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript, postgresql
Domain
backend, databases
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.