Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Node to exit if the pool is idle #2568

Merged
merged 1 commit into from Jul 27, 2021
Merged

Allow Node to exit if the pool is idle #2568

merged 1 commit into from Jul 27, 2021

Conversation

fluggo
Copy link
Contributor

@fluggo fluggo commented Jul 2, 2021

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
Copy link
Collaborator

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
Copy link
Contributor Author

fluggo commented Jul 2, 2021

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
Copy link
Contributor Author

fluggo commented Jul 5, 2021

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
Copy link
Contributor Author

fluggo commented Jul 14, 2021

Is there anything further that needs to be done here?

@brianc
Copy link
Owner

brianc commented Jul 14, 2021

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
Copy link
Owner

brianc commented Jul 16, 2021

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
Copy link
Contributor Author

fluggo commented Jul 16, 2021

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
Copy link
Owner

brianc commented Jul 17, 2021

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
Copy link
Contributor Author

fluggo commented Jul 17, 2021

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

Patch updated.

Based on the suggestion from #2078. This adds ref/unref methods to the
Connection and Client classes and then uses them to allow the process to
exit if all of the connections in the pool are idle. This behavior is
controlled by the allowExitOnIdle flag to the Pool constructor; it defaults
to the old behavior.
@fluggo
Copy link
Contributor Author

fluggo commented Jul 24, 2021

Are we good to go on this one?

@brianc
Copy link
Owner

brianc commented Jul 24, 2021 via email

@brianc
Copy link
Owner

brianc commented Jul 27, 2021

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

@brianc
Copy link
Owner

brianc commented Jul 27, 2021

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

@brianc brianc merged commit 684cd09 into brianc:master Jul 27, 2021
@fluggo
Copy link
Contributor Author

fluggo commented Jul 27, 2021

Thanks! Excited to incorporate it into my projects!

@brianc
Copy link
Owner

brianc commented Jul 27, 2021 via email

@vitaly-t
Copy link
Contributor

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

@ericfortis
Copy link

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
Copy link
Contributor Author

fluggo commented Aug 6, 2021

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
Copy link

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
Copy link
Contributor Author

fluggo commented Aug 7, 2021

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

fluggo commented Nov 19, 2021

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

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.

None yet

6 participants