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

Feature: query timeout #1713

Closed
juliusza opened this issue Sep 3, 2018 · 8 comments
Closed

Feature: query timeout #1713

juliusza opened this issue Sep 3, 2018 · 8 comments

Comments

@juliusza
Copy link

juliusza commented Sep 3, 2018

I want to start a discussion about built in query timeout.

In our production environment, we encountered a problem with TCP connections getting stuck. Knex pool would fill up with connections that are not responsive. Note that this would be different from statement timeout, where DB decides to terminate query. Instead pg client would decide that connection is unresponsive and emit an error.

This situation can be simulated with firewall rule:

sudo iptables -A INPUT -j DROP -p tcp --destination-port 5432

Or just use this code snippet:

const { Client } = require('pg')
const client = new Client({user: "user", password: "pass", database: "db"})
const util = require('util');
const exec = util.promisify(require('child_process').exec);

async function main() {
   await exec("sudo iptables -D INPUT -j DROP -p tcp --destination-port 5432");
   await client.connect()

   console.log("Client connected");
   await exec("sudo iptables -A INPUT -j DROP -p tcp --destination-port 5432");

   console.log("Now waiting for DB to respond... forever")
   const res = await client.query('SELECT $1::text as message', ['Hello world!'])
   console.log(res.rows[0].message) // Hello world!
   await client.end()
   process.exit(0)
}

main()

A possible implementation can be found here: master...juliusza:query_timeout
I know I need to add tests and also edit the native driver code for a proper PR. But first I'm looking for feedback from the community to assert if such a thing would be useful.

Also perhaps

socket.setTimeout(3000);

could be a viable solution. I need to look into that.

@abenhamdine
Copy link
Contributor

Happy to see that !

Do you plan to add tests and send a PR ?

@edevil
Copy link
Contributor

edevil commented Oct 29, 2018

Yes, please!

@edevil
Copy link
Contributor

edevil commented Oct 31, 2018

@juliusza You need help with the PR?

@brianc Would this be merged?

@brianc
Copy link
Owner

brianc commented Oct 31, 2018

I'm definitely down for a pull request for this feature. I think it'd be helpful & something the library should provide since it's pretty difficult to implement in your own code.

@edevil
Copy link
Contributor

edevil commented Nov 7, 2018

@brianc I’ve submitted a PR for this. #1760

@neylsongularte
Copy link

Same problem with me, my temporaly solution:

        var socket = new require('net').Socket();
        socket.setTimeout(20000)

        const client = new pg.Client({
            connectionString: urlPostgres,
            stream: socket
        });

        socket.on('timeout', function () {
            // console.log('timeout')
            client.end();
        });

        await client.connect()

@vitaly-t
Copy link
Contributor

vitaly-t commented Nov 30, 2018

Can this be closed now or there is still a problem with the sockets?

@brianc
Copy link
Owner

brianc commented Nov 30, 2018

yay!

@brianc brianc closed this as completed Nov 30, 2018
harrykao added a commit to harrykao/sequelize that referenced this issue Oct 17, 2022
The `query_timeout` feature of the `pg` package helps handle stuck TCP
connections more quickly and gracefully by implementing a client-side
timeout:

brianc/node-postgres#1713

Sequelize started passing this dialect-specific option through to `pg` here:

sequelize#13258

I believe we also want to invalidate the connection when a client-side
timeout occurs. We shouldn't try to reuse the stuck connection
because...it's stuck.

This PR updates the error handling code so that the connection is
invalidated if the error matches the one thrown from here:

https://github.com/brianc/node-postgres/blob/5538df6b446f4b4f921947b460fe38acb897e579/packages/pg/lib/client.js#L529
WikiRik added a commit to sequelize/sequelize that referenced this issue Oct 24, 2022
* Invalidate connection after client-side timeout.

The `query_timeout` feature of the `pg` package helps handle stuck TCP
connections more quickly and gracefully by implementing a client-side
timeout:

brianc/node-postgres#1713

Sequelize started passing this dialect-specific option through to `pg` here:

#13258

I believe we also want to invalidate the connection when a client-side
timeout occurs. We shouldn't try to reuse the stuck connection
because...it's stuck.

This PR updates the error handling code so that the connection is
invalidated if the error matches the one thrown from here:

https://github.com/brianc/node-postgres/blob/5538df6b446f4b4f921947b460fe38acb897e579/packages/pg/lib/client.js#L529

* Add comment with link to PR.

* Add tests, shorten comment.

* Skip tests for postgres-native.

* Update src/dialects/postgres/query.js

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>

Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
harrykao added a commit to harrykao/sequelize that referenced this issue Nov 15, 2022
Merge ff43e8d from main:

The `query_timeout` feature of the `pg` package helps handle stuck TCP
connections more quickly and gracefully by implementing a client-side
timeout:

brianc/node-postgres#1713

Sequelize started passing this dialect-specific option through to `pg` here:

sequelize#13258

I believe we also want to invalidate the connection when a client-side
timeout occurs. We shouldn't try to reuse the stuck connection
because...it's stuck.

This PR updates the error handling code so that the connection is
invalidated if the error matches the one thrown from here:

https://github.com/brianc/node-postgres/blob/5538df6b446f4b4f921947b460fe38acb897e579/packages/pg/lib/client.js#L529
WikiRik pushed a commit to sequelize/sequelize that referenced this issue Nov 15, 2022
* fix(postgres): invalidate connection after client-side timeout

Merge ff43e8d from main:

The `query_timeout` feature of the `pg` package helps handle stuck TCP
connections more quickly and gracefully by implementing a client-side
timeout:

brianc/node-postgres#1713

Sequelize started passing this dialect-specific option through to `pg` here:

#13258

I believe we also want to invalidate the connection when a client-side
timeout occurs. We shouldn't try to reuse the stuck connection
because...it's stuck.

This PR updates the error handling code so that the connection is
invalidated if the error matches the one thrown from here:

https://github.com/brianc/node-postgres/blob/5538df6b446f4b4f921947b460fe38acb897e579/packages/pg/lib/client.js#L529

* fix syntax error

* fix another syntax error

* fix import
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

No branches or pull requests

6 participants