Perhaps you should also add something like:

const client2 = await pool.connect();
try {
  // Verify that `client` was removed from the pool and replaced w/ a new client
  expect(client2).to.not.equal(client);
} finally {
  await client2.release();
}

(Otherwise, it's possible that client just magically fixed itself through some other means. So, adding this check helps to rule out that possibility)

yah that's not a bad idea! I'd like to hold off on merging this until I get some confirmation its actually fixing things in aws lambda. It also unfortunately doesn't work w/ the native bindings yet so I'll need to somehow figure that out.

Sounds good.

As I mentioned in this thread, I suspect that both "pg" and "knex" were encountering the same issue within their respective pool implementations. So, we'll most likely need to implement a similar fix on the "knex" side.

In the current logic: if it notices that the next-available client is already dead, then it removes it from the pool. Afterwards, it instantiates a new client to replace it.

Would there be any advantage to looping on the client eviction step? In pseudo-code:

while(this._idle.length) {
  const client = this._idle.pop();

  // ... Other important details I'm ignoring for the moment...

  if(isDead(client)) {
    this.remove(client);
  } else {
    return client
  }
}

if(!this._isFull()) {
  return this.newClient(...);
}

throw new Error('unexpected condition');

This seems like it helps in 2 ways:

1. It clears out the dead clients more quickly, and
2. It avoids the need to wait for this.newClient(..) when there is already another client available.

probably, yeah - good suggestions!

hi guys, I want to test this branch, but npm says its missing the version... tried to fork it, put the version (https://github.com/592da/node-postgres) and then compiling failed : Cannot find module 'pg'

@brianc

Hmm... I suspect that's because this is a mono-repo that is built via Lerna . You might need to build it locally first and then link to the build artifacts. (@brianc might know another approach. I haven't actually worked with this codebase before).

I don't remember the exact build steps for Lerna off the top of my head, but I think it's something like:

1. Install Lerna
2. Navigate to the root of the repo
3. Run the command: lerna bootstrap

hi guys, I want to test this branch, but npm says its missing the version... tried to fork it, put the version (https://github.com/592da/node-postgres) and then compiling failed : Cannot find module 'pg'

if you wanna test it it's probably easiest to just apply the diff of the 1 file i changed inside your node_modules/pg-pool folder to index.js (or just copy/paste the changes).

thanks for your reply @briandamaged, unfortunately, it didn't work, maybe because its also imported from typeorm

@brianc , having the same issue...

2020-02-26T21:38:37.302Z	da5bbf66-06b7-4c69-9c85-ab84b1b777a1	INFO	Error: Connection terminated unexpectedly
    at Connection.<anonymous> (/var/task/src/rest/webpack:/node_modules/pg/lib/client.js:255:1)
    at Object.onceWrapper (events.js:312:28)
    at Connection.emit (events.js:223:5)
    at Connection.EventEmitter.emit (domain.js:475:20)
    at Socket.<anonymous> (/var/task/src/rest/webpack:/node_modules/pg/lib/connection.js:139:1)
    at Socket.emit (events.js:228:7)
    at Socket.EventEmitter.emit (domain.js:475:20)
    at endReadableNT (_stream_readable.js:1185:12)
    at processTicksAndRejections (internal/process/task_queues.js:81:21)```

I'm seeing some mention online that stream.readyState is deprecated/unsupported. It might be safer to do this instead:

(client.connection.stream.readable && client.connection.stream.writable)

When experimenting w/ things locally, I'm finding that these properties change as soon as you call stream.end().

yeah those properties don't exist in all supported versions of node that pg supports. stream.readyState does exist in all supported versions.

When experimenting w/ things locally, I'm finding that these properties change as soon as you call stream.end().

you should check if readyState changes as well (I think it does) - reason is any official patch is gonna need to work in node 8, 10, 12, and 13. So if its "officially deprecated" in node but still works, I'd use it (as it probably wasn't deprecated in older versions of node). Typically I do that, write a bunch of tests for the feature, and then if a test breaks on an edge release of node I either decide to drop old version support at that point or make a compatibilty branch in the code that will handle both old and new versions of node.

Sure -- I'll give that a try after the initial experiment. Speaking of which: you can find the experiment within the knex pooling here:

https://github.com/briandamaged/knex/tree/experiment/aws-disconnections

And, here's the link to the specific commit:

briandamaged/knex@1490611

So, I think this roughly aligns w/ your implementation. (Aside from the readyState piece)

yah totally actually I don't wanna burden you w/ backwards compatiblity - if you find a way to make it work in lambda w/ a version of node, i'll do my best to back port it and "production-ify" it for this library. Mostly it's just hard for me to isolate & test since I don't have a lambda nor much experience w/ them so me setting all that testing infrastrucutre up is a huge time sync. But if you figure out the root cause, I'll help any way I can getting a fix out

Ah -- yeah, I currently don't have a way to recreate the issue, either. I just know that Knex's current implementation relies upon Events to detect the closed connections. So, this just seems like a possible way to work around that issue.

Hopefully we'll hear some good news from this experiment! If so, then we can figure out how to formulate the concepts into something that is production-worthy. (One thought: we might even need to swap out implementations depending upon Node version. Ex: use readable / writable if the Node version supports those properties; otherwise, fall back to readyState)