Skip to content

Connection terminated unexpectedly does not propagate to the active query #1700

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

Closed
gajus opened this issue Jul 29, 2018 · 4 comments
Closed
Labels

Comments

@gajus
Copy link
Contributor

gajus commented Jul 29, 2018

Referring to this code:

https://github.com/brianc/node-postgres/blob/master/lib/client.js#L179-L188

First of all, it is not clear how to replicate this error.

Under what circumstances will the end event be triggered without calling .end() on the client?

I have tried:

const connection = await pool.connect();

await connection.query('SELECT pg_sleep(10)');

end then literally terminating the Internet connection, but that results in ETIMEDOUT rather than "Connection terminated unexpectedly".

Point is, this is happening in production application.

I read the notes at https://node-postgres.com/api/pool:

When a client is sitting idly in the pool it can still emit errors because it is connected to a live backend. If the backend goes down or a network partition is encountered all the idle, connected clients in your application will emit an error through the pool's error event emitter. The error listener is passed the error as the first argument and the client upon which the error occurred as the 2nd argument. The client will be automatically terminated and removed from the pool, it is only passed to the error handler in case you want to inspect it.

It is important you add an event listener to the pool to catch errors. Just like other event emitters, if a pool emits an error event and no listeners are added node will emit an uncaught error and potentially exit.

Having read this, it is still unclear to me whats the reason this error cannot be propagated to the active query?

Perhaps I am overlooking something, but was the intention to structure the code as:

if (this.activeQuery) {
      // ..
- }
- if (!this._ending) {
+ } else if (!this._ending) {

?

This fixes the issue ("Connection terminated" propagates to the query), though I am uncertain what are the side effects.

gajus added a commit to gajus/slonik that referenced this issue Jul 29, 2018
If there is no “error” listener defined, and an “error” event is emitted, the error is thrown and the node process exists.

This behaviour is documented in Node.js EventEmitter

https://nodejs.org/api/events.html#events_error_events

In the context of node-postgres, this means that even if there is an active query to associate an error with, when there is no “error” listener defined the program will exit when there is a connection error.

Refer to:

brianc/node-postgres#1700
@charmander
Copy link
Collaborator

Having read this, it is still unclear to me whats the reason this error cannot be propagated to the active query?

It is, because it causes the query to fail. It’s just also emitted on the client, because it’s a client-level error (requires you to connect a new client if not using a pool, for example).

First of all, it is not clear how to replicate this error.

Under what circumstances will the end event be triggered without calling .end() on the client?

Connections being terminated by PostgreSQL, e.g. when shutting down or causing a client’s backend process to exit:

'use strict';

const pg = require('pg');

(async () => {
  const pool = new pg.Pool({
    host: '/var/run/postgresql',
  });

  const client = await pool.connect();

  pool.on('error', error => {
    console.error('error event on pool (%s)', error);
  });

  client.on('end', () => {
    console.log('end event on client');
    pool.end();
  });

  await pool.query('SELECT pg_terminate_backend($1)', [client.processID]);
})();

process.on('unhandledRejection', error => {
  throw error;
});

@gajus
Copy link
Contributor Author

gajus commented Jul 29, 2018

It is, because it causes the query to fail. It’s just also emitted on the client, because it’s a client-level error (requires you to connect a new client if not using a pool, for example).

Problem is that unless client defines "error" event handler, then the Node process is being terminated prior to the query error handler doing clean up.

@charmander
Copy link
Collaborator

Always use a pool, always add an error event listener to the pool. Not emitting the event when there’s an active query doesn’t let you not do this, because it could easily happen when there isn’t an active query.

@gajus
Copy link
Contributor Author

gajus commented Jul 29, 2018

Always use a pool, always add an error event listener to the pool. Not emitting the event when there’s an active query doesn’t let you not do this, because it could easily happen when there isn’t an active query.

That makes sense. It would also make sense then to throw an error if client attempts to create a query without error event listener defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants