client.end() never resolves in Deno when using SSL connections

[SylvainMarty]

Hello there,

First of all, thank you for your work on pg, I used it extensively and it's a great library!

I am currently working on a project with Deno 2, pg, and a database hosted on Neon Postgres.

I ran into a weird issue when calling await client.end():

error: Top-level await promise never resolved
// await client.end();
// ^

This error only happens when connecting to a database using the sslmode=require option.

I had no trouble working on the project with a local Postgres instance.

Neon Postgres only supports SSL connections so this problem is kind of blocking for me.

How to reproduce

I created a repository with minimal code to reproduce the error: https://github.com/SylvainMarty/pg-deno-ssl-issue.

You can also copy-paste the following code in a Deno 2 project:

// @ts-types="npm:@types/pg@8.11.11"
import pg from "npm:pg@8.14.1";

const client = new pg.Client({
  connectionString:
    // CHANGE THIS TO YOUR OWN NEON DB
    "postgresql://user:password@your-own-neon-domain.eu-central-1.aws.neon.tech/neondb?sslmode=require",
});

await client.connect();
await client.query(`SELECT 1`);
await client.end();
// This last line will throw an error:
// error: Top-level await promise never resolved
// await client.end();
// ^

Investigation

Since I was curious, I dug deeper and I realized the error is happening because the event end is never emitted by the Connection class declared in connection.js:

node-postgres/packages/pg/lib/connection.js

Lines 61 to 63 in 477f812

	this.stream.on('close', function () {
	self.emit('end')
	})

To be sure, I added a log in the end() method of the connection.js file:

  end() {
    // 0x58 = 'X'
    this._ending = true
    if (!this._connecting || !this.stream.writable) {
      this.stream.end()
      return
    }
    return this.stream.write(endBuffer, () => {
+     console.log(
+       "event listeners",
+       EventEmitter.getEventListeners(this.stream, 'end'),
+       EventEmitter.getEventListeners(this.stream, 'close'),
+     );
      this.stream.end()
    })
  }

And ran the code again, this time with the log enabled:

// Output from the Node.js runtime:
event listeners [ [Function: onReadableStreamEnd], [Function (anonymous)] ] [ [Function: onSocketCloseDestroySSL] ]

// Output from the Deno runtime:
event listeners [ [Function: onReadableStreamEnd], [] ]

In deed, the close event listener is never called because it doesn't exist.

Why this happens

This problem is due to the fact that Deno's compatibility layer with the Node.js TLS API is not 100% complete yet.

I found the following test suites which are still in the TODO file of the Node compat project:

• parallel/test-tls-close-error.js
• parallel/test-tls-close-event-after-write.js
• parallel/test-tls-close-notify.js

You've read it right, the close event is not emitted like in NodeJS.

I have tried to make those tests pass in Deno/Node compatibility project but without success. There's actually a lot of work to be done on Deno's side for even being able to run those tests correctly, even before actually fixing the issue at hand (Deno doesn't support self signed certificates and that's how NodeJS expects those tests to be running).

I am planning to open an issue on Deno's repo and see if I can help advance on this subject but from what I've seen, the issue is linked to multiple problems and this might not be fixed anytime soon.

Moving forward

Yes I know, everything points to this being more of a Deno scope than pg's.

Now, I think pg can be updated to work around this issue.

Actually, the workaround I found was as simple as this change in pg/lib/connection.js:

  end() {
    // 0x58 = 'X'
    this._ending = true
    if (!this._connecting || !this.stream.writable) {
      this.stream.end()
+     if (this.ssl && Deno?.version) {
+       this.emit('end')
+     }
      return
    }
    return this.stream.write(endBuffer, () => {
      this.stream.end()
+     if (this.ssl && Deno?.version) {
+       this.emit('end')
+     }
    })
  }

I've tested this workaround against the minimal reproducible example provided above, and it successfully resolves the issue. The client.end() call completes properly when using SSL connections to Neon Postgres from Deno.

I'm offering this as a tentative solution and am open to alternative approaches if you have concerns about this implementation.

I saw your comment on the issue #2225 and I understand supporting new runtimes is not a priority for you. My primary goal is to enable Deno users to work with pg and Neon (or other SSL-required Postgres instances) without hitting this blocking issue.

Since pg is a building block for many libraries used extensively by the JS ecosystem (to quote a few: TypeORM, Slonik, pg-boss), I really think it would be great for everyone if this library worked well with Deno 2.

I personally had no trouble working with the pg library on Deno until I found this issue (my project has extensive integration tests and pg is doing a great job at everything we throw at it).

[charmander]

You've read it right, the close event is not emitted like in NodeJS.

It doesn’t sound like there’s anything for pg to do, then. I don’t think it’s appropriate to add specific checks to pg to implement incorrect/inconsistent fallback behavior for an unsupported runtime’s compatibility layer, especially considering there are user-side workarounds like not awaiting the promise returned by client.end() (in the given example).

(You should use sslmode=verify-full, by the way. Pg currently treats sslmode=require as sslmode=verify-full, but relying on that is deprecated.)

[SylvainMarty]

It doesn’t sound like there’s anything for pg to do, then. I don’t think it’s appropriate to add specific checks to pg to implement incorrect/inconsistent fallback behavior for an unsupported runtime’s compatibility layer.

Sadly, I think it is unavoidable because of leaky abstractions. Breaking changes and new APIs are a thing, even between two major LTS versions of NodeJS. Sometime, libraries have to work around their language/runtime quirks to endure.

At the end, the maintainer decide what is going to be the scope of the project he's maintaining, I totally get that. 👍

especially considering there are user-side workarounds like not awaiting the promise returned by client.end() (in the given example).

IMO, not awaiting a promise is not a super good solution in its own because it could cause race conditions. Also, I personnally configure test runners to detect open handles when a unit test/integration test ends and this would definitely trigger them.

Do you think there is a way for me to check if the client's connection is correctly closed from the client object so I can wrap the ending logic in my own promise?

Here is an example of what I mean:

await new Promise((resolve) => {
  client.end();
  let intervalId = setInterval(() => {
    if (!client.isConnectionClosed) return;
    clearInterval(intervalId);
    resolve();
  }, 10);
});

This scenario would be more acceptable for me:

• when my promise is done, there is no more open handles left in my software
• the client.end() function is not called that often so the interval delay is an acceptable tradeoff for my case

(You should use sslmode=verify-full, by the way. Pg currently treats sslmode=require as sslmode=verify-full, but relying on that is deprecated.)

Good to know, thank you for the advice!

[charmander]

IMO, not awaiting a promise is not a super good solution in its own because it could cause race conditions.

In general, yes, but end() specifically should be pretty harmless.

Also, I personnally configure test runners to detect open handles when a unit test/integration test ends and this would definitely trigger them.

It doesn’t look like your proposed library patch would affect that, since it emits 'end' immediately without waiting for the socket to close (this is the incorrect/inconsistent fallback behavior I was referring to).

I haven’t investigated too deeply yet, but it looks like Deno might emit the 'end' event? So maybe you could do:

const {connection} = client;

connection.stream.on('end', () => {
  connection.emit('end');
});

(in pool.on('connect', (client) => { … }) if using a pool), but I’m not sure if an open handle for the purposes of however this open handle detection works would still exist at 'end'.

[SylvainMarty]

Thank you, your solution worked and no open handles is detected by the system. 🥳

If I may add a little feedback: currently, the type of pg.Client class doesn't expose the connection property.
It makes the TS compiler complain and we have to use @ts-expect-error or a cast as you can see in the example below.
This is not ideal because code outside of a class should never rely on a private/hidden property as it make it sensible to breaking changes in the internal implementation.
I noticed Slonik also relies on the same property after brianc advice in #3015 (and also uses @ts-expect-error to tell the compiler to not panic).
If the connection property is useful in many cases, it would be nice to update the pg.Client type to officially expose its connection property to the outside world.

---

For people looking to fix the same problem, here's how you can fix it:

import pg from "pg";

async function endClientConnection(client: PgClient) {
  if (client.ssl) {
    // @ts-expect-error: connection is not exposed by `pg` type definitions
    const ( connection } = client;
    connection.stream.on('end', () => {
      connection.emit('end');
    });
  }
  await client.end();
}

For people which have the same problem using Slonik with Deno, you will have to implement your own PG driver factory:
https://github.com/lopkol/sff-vektor/blob/a08f6b303295775443efcaf8a5050e627015c82c/slonik-pg-driver/create_pg_driver_factory.ts#L199-L206

Edit: update example to use @ts-expect-error instead of cast

[charmander]

It’s not a private or hidden property; the TypeScript typings for pg just aren’t complete (and are not currently part of this repo). So feel free to send PRs to DefinitelyTyped as necessary.

[brianc]

I wanted to say thanks for sunch a comprehensive issue report as this w/ an enormous amount of investigation & thought before opening it. It's a rare treat to read something like that. Sorry for not seeing it sooner. I'm trying to do a new thing where I keep my github streak going as much as possible so should hopefully be more engaged. I'm not sure anything needs to be done now as it looks like a solution was met but, thanks anyway!