Allow Node to exit if the pool is idle

[fluggo]

Based on the suggestion from #2078.

Unfortunately I don't yet know how to test this under Mocha, as the whole point is to let Node exit, but I did test it with the following script:

const Pool = require('./index')

const pool = new Pool({ idleTimeoutMillis: 1000 })
pool.query('SELECT NOW()', (err, res) => console.log(res))
pool.on('remove', () => {
  console.log('removed')
  done()
})

setTimeout(() => {
  pool.query('SELECT * from generate_series(0, 1000)', (err, res) => console.log(res))
}, 500)

The program waits for the idle timeout during the time between the first query and the second, but exits immediately after the second query has finished running. After the second query, Node's loop is empty, and since the idle timeout and the one connected socket are unref'd, Node can exit right away.

If anyone has any suggestions on how to test this under Mocha, I'm all ears.

[charmander]

I wonder if it’d be better to introduce client.ref() and client.unref() and have the pool use those?

If anyone has any suggestions on how to test this under Mocha, I'm all ears.

Maybe by spawning new processes?

[fluggo]

I wonder if it’d be better to introduce client.ref() and client.unref() and have the pool use those?

I thought about this, but I didn't know how to test across the packages.

If anyone has any suggestions on how to test this under Mocha, I'm all ears.

Maybe by spawning new processes?

Good idea. I'll give it a shot.

[fluggo]

All done, ref/unref now appear on Client and Connection, and pg-pool uses them to maintain the pool. There is now also a Mocha test.

HOWEVER 📢🧨️🚨 this now causes a new relationship between the next version of pg-pool and the next version of pg. pg-pool will need to depend on the version of pg that has the ref/unref in it.

[fluggo]

Is there anything further that needs to be done here?

[brianc]

Is there anything further that needs to be done here?

I just need to take a look at this & get it merged & released! Should be able to look here today or tomorrow!

[brianc]

okay I checked this out...looks pretty good! I have a concern though - because this is a breaking change I'll need to rev a new semver major to release this in its current state. I like to batch up breaking changes and limit the number of times I do that. Would it be possible to make this an optional config on the pool like const pool = new Pool({ allowExitOnIdle: true }) or something, and default it to false (existing behavior) - that way we can release this in a semver minor as soon as it's ready.

[fluggo]

Is this a breaking change? It would be a very odd program that would fall prey to this—it would have to be relying on the connection pool's idle timer to keep the program alive while another unref'd timer or socket was doing work. I don't think you've made a guarantee that an idle connection pool will keep the process alive.

[brianc]

Yeah...I know what you're saying 100%. There's this synthesizer I make music with sometimes called the deluge. It's a pretty incredible little device developed by just two folks from New Zealand. One of it's features is when the CPU gets overloaded on the device instrument voices drop from playback (istead of distorting or stopping playback). The lead developer says he has to semver the releases when he makes the code more CPU efficient because people have compositions which accidentally rely on the CPU overload protection voice dropout, when the code gets better their music changes and they aren't happy. pg has been installed 1.75 million times this week, and most likely there are at least a handful of programs which accidentally rely on the pool's idle timeout keeping their program alive to allow some other unreffed thing to complete in the background. I'm okay adding the flag myself if you want to step away from this work for now - I'm already grateful for your contribution...but yeah I'd consider this a silent, sneaky, incredibly hard to debug "breaking" change to people who are relying on unspecified but up until now steady behavior of the library. I'm also fine putting this behind a flag w/ the original behavior "off" and the flipping the default to "on" in the next semver major release. With the size of the install base of this library for better & for worse I try to above most everything (outside security and performance) maintain backwards compatibility of everything outside of semver major bumps.

[fluggo]

I had a feeling I wouldn't win that argument, but I thought I'd try! 😆

Patch updated.

[fluggo]

Are we good to go on this one?

[brianc]

Yes! I was visiting parents this week out of state. Flying home today, will merge + rlease tomrrow!

…

On Sat, Jul 24, 2021 at 5:39 AM Brian Crowell ***@***.***> wrote: Are we good to go on this one? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2568 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIPNAID23UFPZELS723TZKYA7ANCNFSM47XJ3SJA> .

[brianc]

will be releasing this this morning - sorry for the delay & thanks so much for the PR!

[brianc]

will be releasing this this morning - sorry for the delay & thanks so much for the PR!

err one question I put above before I get this out

[fluggo]

Thanks! Excited to incorporate it into my projects!

[brianc]

released! sorry for the delay!

…

On Tue, Jul 27, 2021 at 11:56 AM Brian Crowell ***@***.***> wrote: Thanks! Excited to incorporate it into my projects! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#2568 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIOO4BQPGPRGDJKHEM3TZ3QKJANCNFSM47XJ3SJA> .

[vitaly-t]

Wonderful update, finally, kudos to @fluggo! I have updated pg-promise to support it.

[ericfortis]

Yeah...I know what you're saying 100%. There's this synthesizer I make music with sometimes called the deluge. It's a pretty incredible little device developed by just two folks from New Zealand. One of it's features is when the CPU gets overloaded on the device instrument voices drop from playback (istead of distorting or stopping playback). The lead developer says he has to semver the releases when he makes the code more CPU efficient because people have compositions which accidentally rely on the CPU overload protection voice dropout, when the code gets better their music changes and they aren't happy. pg has been installed 1.75 million times this week, and most likely there are at least a handful of programs which accidentally rely on the pool's idle timeout keeping their program alive to allow some other unreffed thing to complete in the background. I'm okay adding the flag myself if you want to step away from this work for now - I'm already grateful for your contribution...but yeah I'd consider this a silent, sneaky, incredibly hard to debug "breaking" change to people who are relying on unspecified but up until now steady behavior of the library. I'm also fine putting this behind a flag w/ the original behavior "off" and the flipping the default to "on" in the next semver major release. With the size of the install base of this library for better & for worse I try to above most everything (outside security and performance) maintain backwards compatibility of everything outside of semver major bumps.

I see this is merged, but isn't calling pool.end() faster and better?

---

Also that story deprecates xkcd 1172

---

[fluggo]

I see this is merged, but isn't calling pool.end() faster and better?

Faster? I doubt it. pool.end() does the same cleanup Node itself would have to do before the program exits. (My previous explanation here was wrong; Node has teardown logic for handles.)

Better? Depending on your programming style, you might think of pool.end() as cleaner, as you're forcing yourself to have a known path toward your program's exit so that you can call this function once and only after all tasks have completed. I'm getting older and deciding not to be so masochistic. I have several programs where it's not clear when the right time is to call pool.end(). For example, I have servers that keep handling existing connections after I close the server's listener, and there's no callback to tell me when those complete. I also have scheduled tasks and utility programs that call into the pool, not to mention the regression tests, and rather than having to figure out the correct point in each program to call pool.end() (and debugging and debugging when I get it wrong), I'd much rather just have a pool that lets me exit naturally: when everything else I care about is done.

This has a precedent in several Node.js APIs. Sockets and timeouts can be unref'd to let the process exit naturally in spite of their existence, and the http.Agent object unrefs all of its idle sockets without asking—to do otherwise would require you to manually clean up even the default Agent after any HTTP calls, which I think breaks the feel of Node.js as a scripting runtime.

[ericfortis]

I have servers that keep handling existing connections after I close the server's listener...

OK. The feature is to gracefully stop servers without a cluster

not to mention the regression tests

FWIW, in Mocha, it's possible to:

function serveUntilDone() {
  const server = http.createServer(router);
  before(done => {
    initDatabasePool();
    server.listen(serverAddr, done);
  })
  after(done => {
    db.pool.end();
    server.close(done);
  })

[fluggo]

Yes, I have that same code in my mocha tests. I still have trouble getting it to exit gracefully. I'm looking to keep the hair I have.

If you have a point in favor of making this harder to use than http.Agent, say so.

[ericfortis]

If you have a point in favor of making this harder to use than http.Agent, say so.

I just wanted to know if there was something I needed to worry about this feature.

[karlbecker]

Just wanted to add a note here saying thank you for avoiding making this a breaking feature @fluggo - and for the nice story about the synthesizer @brianc 😊

With the size of the install base of this library for better & for worse I try to above most everything (outside security and performance) maintain backwards compatibility of everything outside of semver major bumps.

Thank you x 1000

[fluggo]

Oh hey! And it's in the docs now! Kudos!