Node-mssql: How/where to handle ESOCKET/ECONNRESET error

[MatejSkrbis]

I tried to listen for error event, but as I saw in source code (tedious.js), ESOCKET event is ignored and nothing is emitted. I could see ECONNRESET in debug however. So how can I handle this event correctly? Occasionally this error occurs and then all requests are time-outed.

tedious.js

tedious.on('error', err => {
  if (err.code === 'ESOCKET') {
    tedious.hasError = true
    return
  }

  this.emit('error', err)
})

if (this.config.debug) {
  tedious.on('debug', this.emit.bind(this, 'debug', tedious))
}

[dhensby]

I looked into this as part of my work on #806 and tried to remove the strange ESOCKET handling (as I found this confusing too).

However I found that doing so led to unhandled errors crashing node when trying to connect to an offline DB. I presume this is because the errors are emitted before an error handler can be attached to the pool during the initial "probing" connection.

It would be good to pass these errors on somehow, but I didn't spend too much time investigating.

I'm going to leave this open as it is a legitimate issue

[dhensby]

I've spent some time looking into this a bit more and it's not completely clear to me why the ESOCKET errors are not emitted.

We can attach error handlers before they are emitted, so that's not the problem.

The only things I can think are:

1. We don't want socket errors being emitted as an event and causing a rejection of the _poolCreate promise because the promise should be handled itself
2. We don't want socket errors (ie: timeouts of connections) being emitted when connections are just hanging around in the pool, connections might suffer socket errors in the background and not be of any consequence to the application.

However, both these instances can be handled successfully with a stringent enough error handler.

@patriksimek - I'd really like your insight here.

---

It was introduced as a fix to https://github.com/tediousjs/node-mssql/issues/234 (https://github.com/tediousjs/node-mssql/commit/0891182293a30ea148baab1d698a26b68929ecae)

[patriksimek]

Unfortunately, I can't remember what was the idea behind ignoring the ESOCKET error. Judging by looking into the code, I marked the connection with hasError flag and was expecting the node-pool to destroy that connection for me. As you mentioned - the current code is probably not stringent enough to handle various connection states correctly.

[katlimruiz]

Well I think I can give some ideas. Actually I've been trying to research how to handle those ESOCKET errors and I finally found to catch those on the pool.on('error') event.

However, these three errors seem to not be "errors":

'Connection lost - write ECONNRESET'
'Connection lost - read ECONNRESET'
'Connection lost - socket hang up'.

These are mainly events notifying that one of those connections was closed to idle timeout or alike (I'm pretty sure, though not 100%).

So if you are saying these will not show up anymore, then I guess that is ok, however, I now would be in doubt whether these are false positives, or real errors to get notified about.

I would add a flag somewhere in the event arguments, to know where it came from (ECONNRESET should be a good example of a source that you can ignore).

This would be my take on this:

            pool.on('error', err => {
                switch (err.message) {
                // NOTE: these are known errors when the connections simply go idle, in theory mssql.ConnectionPool should handle
                // reconnecting and expanding the pool when necessary.
                case 'Connection lost - write ECONNRESET':
                case 'Connection lost - read ECONNRESET':
                case 'Connection lost - socket hang up':
                    koa.log.debug('mssql: pool connection reset', err);
                    break;
                default:
                    koa.log.error('mssql: pool event error', err);
                }
            });

[dhensby]

But pool errors are quite different from listening to an error on a specific connections for the period you are using it, right? Isn't that what we are talking about?

[ssontag55]

I this error when running larger queries, queries run fine in sql but lose connection when running through Transaction or Request RequestError: Connection lost - read ECONNRESET

keepactive:true helps

[rubenstolk]

@ssontag55 where do you add keepactive: true, haven't found documentation for that. We're getting this error after upgrading from Node 11 to 12...

[ssontag55]

I think I found it by happenstance, I seem to be having some success with this setup:

`connectionLimit : 50,

connectionTimeout: 32000,

requestTimeout: 32000,

pool: {

    idleTimeoutMillis: 32000

    //max: 100
},

stream: true,

parseJSON: true,

options: {

    database: '', 

    encrypt: false,

    keepAlive:true,

    rowCollectionOnRequestCompletion: true

}`

[rubenstolk]

I've upgraded to v6.0.1 and that seems to have fixed my issue, will keep monitoring it for now. Unfortunately the release/changelog management is not totally clear. @dhensby is that something that can be improved please?

[dhensby]

What aspects, @rubenstolk

[rubenstolk]

@dhensby changelog is 8 months old, I don’t know when 6.0.0 was released and no release notes for 6.0.1 ...

[dhensby]

@rubenstolk https://github.com/tediousjs/node-mssql/pull/953

[katesclau]

@ssontag55 keepAlive didn't help us.

Socket hangs after 30mins, and we promisify the connection, using a single one to handle all requests

ie.:

 export default class PromisedConnection {
  private connection: Promise<Connection> | null;
  private config: ConnectionConfig;

  constructor(config: ConnectionConfig) {
    // this.connection always points to a promise resolving after the last assigned request
    // initial setting is either resolved once a new connection is established or rejected if an error occurs
    this.config = config;
    this.connection = this.getConnection();
  }

   public async getConnection() {
    return this.connection || new Promise((resolve, _reject) => {
      const dbConnection = new Connection(this.config);
      dbConnection.on('connect', (err: any) => {
        if(err) {
          logger.error('Failed to connect to Azure SQL.', extractError(err));
          resolve();
        } else {
          resolve(dbConnection);
        }
      }).on('error', (err: any) => {
        logger.warn('Connection lost: ', extractError(err));
        this.connection = null;
        this.getConnection();
      });
    });
  }

  public async request(query: string, parameters: any[] = [], options: IRequestOptions) {
...

Our current config:
{ server: getEnv('HOST'), authentication: { type: 'default', options: { userName: getEnv('DB_USER'), password: getEnv('DB_USER'), }, }, connectionTimeout: 32000, requestTimeout: 32000, connectionLimit: 50, pool: { idleTimeoutMillis: 32000, }, options: { useColumnNames: true, rowCollectionOnDone: true, rowCollectionOnRequestCompletion: true, parseJSON: true, stream: true, debug: { packet: false, data: true, payload: true, token: false, log: false, }, keepAlive: true, database: getEnv('DB'), encrypt: true, }, }

"tedious": "6.3.0",
node: v10.15.3

[dhensby]

@katesclau I think you're in the wrong repo, this is for node-mssql, not the tedious driver

[flacohenao]

Btw, even if we handle that, would be very usefull to add a reconnection script to the docs...

like, when you get those errors on the first handshake, wouldnt be nice to autoreconnect the DB?

[dhensby]

I don't think its for the library to be making calls on when to try to reconnect to the DB. If the initial probe connection fails there's no reason to try again. When new connections are added to the pool they try to connect and if they fail the connection is discarded and a new one obtained until the pool is depleted of working connections. If no new connections can be established the pool becomes "unhealthy". It's up to the developer to decide how to handle unhealthy pools or individual connections that go bad.

I'm not sure this issue needs to be left open, I think it's being handled as expected now.