abhijeet-pandhe commented 3 years ago

Please answer these questions before submitting your issue. Thanks!

What version of NodeJS are you using (node --version and npm --version)? NodeJS: v12.22.1 npm: 6.14.12
What operating system and processor architecture are you using? No LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 18.04.5 LTS Release: 18.04 Codename: bionic
What are the component versions in the environment (npm list)? snowflake-sdk@1.6.1
What did you do? I'm building a custom connection pool for the Snowflake node connector. I couldn't find any method in Snowflake docs to check if the connection is still valid or not. Looking through the 'snowflake-sdk' library I found a method named isUp() which says it "returns true if the connection is active otherwise false". I used this isUp() method to validate the connection before returning it to my query worker which will execute the query.
What did you expect to see? I expect isUp() to return true if the connection is active otherwise false (even if the connection is terminated by Snowflake i.e. not destroyed from my code)
What did you see instead? While testing this I found out that isUp() returns true if the connection is kept ideal until it's terminated from the Snowflake side (i.e. not destroyed by my code). Due to this, my connection pool thinks it is still a valid connection and returns it to my query worker. And by querying using this connection I get "Unable to perform operation using terminated connection" error.

Is there any correct way/method for checking if the connection is still valid?

MadhuPolu commented 2 years ago

Here I'm posting my workaround, and hope that it'll be helpful for someone who will be looking for the same.



private connection = this.getConnection();
private retryWaitTime = 300;

getConnection() {
    return snowflake.createConnection({
      ...
      ...
    });
  }

async getQueryResults<T>(sqlText: string) {
    const conn: any = await this.getEstablishedConnection();
    const rows: T = await new Promise((res, rej) => {
      conn.execute({
        sqlText,
        complete: (err, stmt, rows) => {
          if (err) {
            if (
              err.code === 407002 ||
              err.message ===
                'Unable to perform operation using terminated connection.'
            ) {
              /**
               * Workaround for this issue: https://github.com/snowflakedb/snowflake-connector-nodejs/issues/168
               */
              setTimeout(async () => {
                const _rows = await this.getQueryResults<T>(sqlText);
                res(_rows as T);
              }, this.retryWaitTime); // No chance for infinite looping here as it's not going to happen with in the getEstablishedConnection as per Today's code - Just double checking whether this needs #retrycounter or not
            } else {
              console.error(
                'Failed to execute statement due to the following error: ' +
                  err.message,
              );
              rej(err);
            }
          } else {
            res(rows);
          }
        },
      });
    });
    return rows || [];
  }

  async getEstablishedConnection() {
    const resolvedActiveConnection = await new Promise((res, rej) => {
      this.connection?.isUp() || false
        ? res(this.connection) // Serve the existing active connection if any since the service provider (ChartsService) singleton by default
        : this.connection.connect(
            (err: Error & { code: number; sqlState: number }, _connection) => {
              // Establish connection
              if (err) {
                // console.log('name: ', err.name);
                // console.log('message: ', err.message);
                // console.log('code: ', err.code);
                // console.log('sqlState: ', err.sqlState);
                // console.log('error: ', Object.keys(err));
                if (
                  err.code === 405501 ||
                  err.message === 'Connection already in progress.'
                ) {
                  console.info('405501 message: ', err.message);
                  setTimeout(() => {
                    res(this.getEstablishedConnection());
                  }, this.retryWaitTime); // No chance for infinite looping here as connection already in progress - Just double checking whether this needs #retrycounter or not
                } else if (
                  err.code === 405503 ||
                  err.message ===
                    'Connection already terminated. Cannot connect again.'
                ) {
                  console.info('405503 message: ', err.message);
                  this.connection = this.getConnection(); // Old connection can't be established, once existing connection gets terminated
                  setTimeout(() => {
                    res(this.getEstablishedConnection());
                  }, 0); // No chance for infinite looping here as new connection is created before calling this - Just double checking whether this needs #retrycounter or not
                  // if (this.connection.isUp()) {
                  //   this.connection.destroy((err, conn) => {
                  //     if (err) {
                  //       console.error('Unable to disconnect: ' + err.message);
                  //     } else {
                  //       console.log(
                  //         'Disconnected connection with id: ' +
                  //           this.connection.getId(),
                  //       );
                  //       setTimeout(() => {
                  //         res(this.getEstablishedConnection());
                  //       }, this.retryWaitTime);
                  //     }
                  //   });
                  // }
                } else {
                  console.error(`Unable to connect, error: ${err.message}`);
                  rej(err);
                }
              } else {
                console.log('Successfully connected to Snowflake.');
                res(_connection);
              }
            },
          );
    });
    return resolvedActiveConnection;
  }
}```

leoromanovsky commented 2 years ago

Hey @MadhuPolu thanks a lot for posting this! Unrelated but if you are wanting to format code and preserving spaces you can wrap it in three backticks at start and end:

export enum ocspModes {
    FAIL_CLOSED = 'FAIL_CLOSED',
    FAIL_OPEN = 'FAIL_OPEN',
    INSECURE = 'INSECURE',
}

MadhuPolu commented 2 years ago

hey @leoromanovsky, just edited by putting the three backticks, thanks for the tip.

varun-dc commented 2 years ago

Ran into this issue as well. Similarly, we're pooling connections. A simple workaround we got working was to perform a simple query first to see if the connection is terminated or not, which if terminated will result in isUp reporting the true status of the connection,

/**
 * Performs a simple query to check if the connection is terminated
 *
 * As a side effect, after this has run `client.isUp()` will report the true
 * status as well -- `isUp` may tell you `true` when in fact it's actually not
 * if you check without running a query first.
 */
function checkConnectionIsTerminated(
  client: snowflake.Connection
): Promise<boolean> {
  return new Promise((resolve) => {
    client.execute({
      sqlText: "select 1",
      complete: (err) => {
        resolve(!!err);
      },
    });
  });
}

livgust commented 2 years ago

This is still an issue for me, can it be re-opened?

OronMeller commented 1 year ago

We had the same issue, setting clientSessionKeepAlive = true in the connectionOptions seems to resolve it:

const options: ConnectionOptions = {
        ...,
        clientSessionKeepAlive: true,
        clientSessionKeepAliveHeartbeatFrequency: 3600,
        ...
      };

const connection: Connection = createConnection(options);