SNOW-1658878 unhandled rejection at promise the callback argument mus…

…t be of type function. received undefined (#939)
snowflakedb · Nov 4, 2024 · c77f3d6 · c77f3d6
1 parent b45ac51
commit c77f3d6
Show file tree

Hide file tree

Showing 9 changed files with 174 additions and 26 deletions.
diff --git a/lib/connection/connection.js b/lib/connection/connection.js
@@ -166,7 +166,7 @@ function Connection(context) {
   };
 
   /**
-   * Callback for connect() used to establish a connection.
+   * Method returning callback function for connect() - used to establish a connection.
    *
    * @param self
    * @param {Function} callback
@@ -176,7 +176,9 @@ function Connection(context) {
   function connectCallback(self, callback) {
     return function (err) {
       if (Parameters.getValue(Parameters.names.CLIENT_SESSION_KEEP_ALIVE)) {
-        self.keepalive = setInterval(self.heartbeat, Parameters.getValue(Parameters.names.CLIENT_SESSION_KEEP_ALIVE_HEARTBEAT_FREQUENCY) * 1000, callback);
+        const SECONDS_TO_MILLISECONDS_MULTIPLIER = 1000;
+        const KEEP_ALIVE_HEARTBEAT_FREQUENCY_IN_MS = Parameters.getValue(Parameters.names.CLIENT_SESSION_KEEP_ALIVE_HEARTBEAT_FREQUENCY) * SECONDS_TO_MILLISECONDS_MULTIPLIER;
+        self.keepalive = setInterval(self.heartbeat, KEEP_ALIVE_HEARTBEAT_FREQUENCY_IN_MS, self);
       }
       if (Util.isFunction(callback)) {
         callback(Errors.externalize(err), self);

diff --git a/lib/connection/connection_config.js b/lib/connection/connection_config.js
@@ -6,6 +6,7 @@ const os = require('os');
 const url = require('url');
 const Util = require('../util');
 const Errors = require('../errors');
+const ConnectionConstants = require('../constants/connection_constants');
 const path = require('path');
 const ErrorCodes = Errors.codes;
 const NativeTypes = require('./result/data_types').NativeTypes;
@@ -381,7 +382,7 @@ function ConnectionConfig(options, validateCredentials, qaMode, clientInfo) {
     Errors.checkArgumentValid(Util.isNumber(clientSessionKeepAliveHeartbeatFrequency),
       ErrorCodes.ERR_CONN_CREATE_INVALID_KEEP_ALIVE_HEARTBEAT_FREQ);
     clientSessionKeepAliveHeartbeatFrequency =
-      Util.validateClientSessionKeepAliveHeartbeatFrequency(clientSessionKeepAliveHeartbeatFrequency, 14400);
+      Util.validateClientSessionKeepAliveHeartbeatFrequency(clientSessionKeepAliveHeartbeatFrequency, ConnectionConstants.HEARTBEAT_FREQUENCY_MASTER_VALIDITY);
   }
 
   const jsTreatIntegerAsBigInt = options.jsTreatIntegerAsBigInt;

diff --git a/lib/constants/connection_constants.js b/lib/constants/connection_constants.js
@@ -0,0 +1 @@
+exports.HEARTBEAT_FREQUENCY_MASTER_VALIDITY = 14400;
diff --git a/lib/parameters.js b/lib/parameters.js
@@ -83,7 +83,7 @@ const parameters =
       {
         name: names.CLIENT_SESSION_KEEP_ALIVE_HEARTBEAT_FREQUENCY,
         value: 3600,
-        desc: 'The amount of time in seconds that a heartbeat will be sent to the server'
+        desc: 'The amount of time (in seconds) between subsequent heartbeat requests to the server.'
       }),
     new Parameter(
       {

diff --git a/test/integration/testConnection.js b/test/integration/testConnection.js
@@ -8,6 +8,7 @@ const assert = require('assert');
 const connOption = require('./connectionOptions');
 const testUtil = require('./testUtil');
 const Util = require('./../../lib/util');
+const nodeUtil = require('util');
 const Core = require('./../../lib/core');
 const { stdout } = require('test-console');
 const { assertLogMessage } = require('./testUtil');
@@ -38,13 +39,14 @@ describe('Connection test', function () {
     const connection = snowflake.createConnection(connOption.valid);
     assert.deepEqual(connection.getTokens(), {});
   });
+
   it('Simple Connect', async function () {
     const connection = snowflake.createConnection(connOption.valid);
 
     await testUtil.connectAsync(connection);
-    assert.ok(connection.isUp(), 'not active');
+    testUtil.assertConnectionActive(connection);
     await testUtil.destroyConnectionAsync(connection);
-    assert.ok(!connection.isUp(), 'still active');
+    testUtil.assertConnectionInactive(connection);
   });
 
   it('Wrong Username', function (done) {
@@ -112,7 +114,7 @@ describe('Connection test', function () {
     timeout();
   });
 
-  it('Failed connections returns sanitized error', function (done) {
+  it('Failed connection returns sanitized error', function (done) {
     const randomId = uuidv4();
     const randomId2 = uuidv4();
     const connection = snowflake.createConnection({
@@ -136,8 +138,52 @@ describe('Connection test', function () {
         }
       });
   });
+
+  it('When connect async with original callback then successfully established', async function () {
+    const connection = snowflake.createConnection(connOption.valid);
+    await testUtil.connectAsyncWithOriginalCallback(connection, () => {});
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
+
+  it('When connect async with undefined callback then successfully established', async function () {
+    const connection = snowflake.createConnection(connOption.valid);
+    await testUtil.connectAsyncWithOriginalCallback(connection, undefined);
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
+
+  it('When connect async with null callback then successfully established', async function () {
+    const connection = snowflake.createConnection(connOption.valid);
+    await testUtil.connectAsyncWithOriginalCallback(connection, null);
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
+
+  it('When connect async within the strict mode then successfully established', async function () {
+    'use strict';
+    const connection = snowflake.createConnection(connOption.valid);
+    await testUtil.connectAsync(connection);
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
+
+  it('When promisify called with call then successfully established', async function () {
+    const connection = snowflake.createConnection(connOption.valid);
+    await nodeUtil.promisify(connection.connect).call(connection);
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
+
+  it('When promisify called with bind then successfully established', async function () {
+    const connection = snowflake.createConnection(connOption.valid);
+    await nodeUtil.promisify(connection.connect.bind(connection))();
+
+    await testUtil.assertActiveConnectionDestroyedCorrectlyAsync(connection);
+  });
 });
 
+
 describe('Connection test - validate default parameters', function () {
   before(() => {
     configureLogger();
@@ -462,7 +508,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise1.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 0);
 
             connectionPool.release(connection).then(() => {
@@ -500,7 +546,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise1.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 4);
 
             connectionPool.release(connection).then(() => {
@@ -512,7 +558,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise2.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 3);
 
             connectionPool.release(connection).then(() => {
@@ -524,7 +570,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise3.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 2);
 
             connectionPool.release(connection).then(() => {
@@ -536,7 +582,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise4.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 1);
 
             connectionPool.release(connection).then(() => {
@@ -548,7 +594,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, release the connection
           resourcePromise5.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 0);
 
             connectionPool.release(connection).then(() => {
@@ -577,7 +623,7 @@ describe('Connection test - connection pool', function () {
 
     // Once acquired, destroy the connection
     resourcePromise1.then(function (connection) {
-      assert.ok(connection.isUp(), 'not active');
+      testUtil.assertConnectionActive(connection);
       assert.equal(connectionPool.pending, 0);
 
       connectionPool.destroy(connection).then(() => {
@@ -610,7 +656,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, destroy the connection
           resourcePromise1.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 4);
 
             connectionPool.destroy(connection).then(() => {
@@ -622,7 +668,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, destroy the connection
           resourcePromise2.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 3);
 
             connectionPool.destroy(connection).then(() => {
@@ -634,7 +680,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, destroy the connection
           resourcePromise3.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 2);
 
             connectionPool.destroy(connection).then(() => {
@@ -646,7 +692,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, destroy the connection
           resourcePromise4.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 1);
 
             connectionPool.destroy(connection).then(() => {
@@ -658,7 +704,7 @@ describe('Connection test - connection pool', function () {
         function (callback) {
           // Once acquired, destroy the connection
           resourcePromise5.then(function (connection) {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.pending, 0);
 
             connectionPool.destroy(connection).then(() => {
@@ -683,7 +729,7 @@ describe('Connection test - connection pool', function () {
     // Use the connection pool, automatically creates a new connection
     connectionPool
       .use(async (connection) => {
-        assert.ok(connection.isUp(), 'not active');
+        testUtil.assertConnectionActive(connection);
         assert.equal(connectionPool.size, 1);
         assert.equal(connectionPool.pending, 0);
         assert.equal(connectionPool.spareResourceCapacity, 4);
@@ -696,7 +742,7 @@ describe('Connection test - connection pool', function () {
         // Use the connection pool, will use the existing connection
         connectionPool
           .use(async (connection) => {
-            assert.ok(connection.isUp(), 'not active');
+            testUtil.assertConnectionActive(connection);
             assert.equal(connectionPool.size, 1);
             assert.equal(connectionPool.pending, 0);
             assert.equal(connectionPool.spareResourceCapacity, 4);
@@ -723,7 +769,7 @@ describe('Connection test - connection pool', function () {
     try {
       // Use the connection pool, automatically creates a new connection
       await connectionPool.use(async (connection) => {
-        assert.ok(connection.isUp(), 'not active');
+        testUtil.assertConnectionActive(connection);
         assert.equal(connectionPool.size, 1);
       });
     } catch (err) {

diff --git a/test/integration/testUtil.js b/test/integration/testUtil.js
@@ -40,6 +40,12 @@ module.exports.connectAsync = function (connection) {
   });
 };
 
+// Other connect-related methods form testUtil do not allow to pass the custom callback from tests
+// This should be used when it is important to execute exactly the specified callback - with no wrapper
+module.exports.connectAsyncWithOriginalCallback = function (connection, callback) {
+  return connection.connectAsync(callback);
+};
+
 module.exports.destroyConnection = function (connection, callback) {
   connection.destroy(function (err) {
     assert.ok(!err, JSON.stringify(err));
@@ -319,5 +325,24 @@ module.exports.createRandomFileName = function ( option = { prefix: '', postfix:
   return fileName;
 };
 
+module.exports.sleepAsync = function (ms) {
+  return new Promise(resolve => setTimeout(resolve, ms));
+};
+
+module.exports.assertConnectionActive = function (connection) {
+  assert.ok(connection.isUp(), 'Connection expected to be active, but was inactive.');
+};
+
+module.exports.assertConnectionInactive = function (connection) {
+  assert.ok(!connection.isUp(), 'Connection expected to be inactive, but was active.');
+};
+
+module.exports.assertActiveConnectionDestroyedCorrectlyAsync = async function (connection) {
+  module.exports.assertConnectionActive(connection);
+  await module.exports.destroyConnectionAsync(connection);
+  module.exports.assertConnectionInactive(connection);
+};
+
+
 module.exports.normalizeRowObject = normalizeRowObject;
 module.exports.normalizeValue = normalizeValue;
diff --git a/test/unit/mock/mock_http_client.js b/test/unit/mock/mock_http_client.js
@@ -39,9 +39,11 @@ MockHttpClient.prototype.request = function (request) {
 
   // Closing a connection includes a requestID as a query parameter in the url
   // Example: http://fake504.snowflakecomputing.com/session?delete=true&requestId=a40454c6-c3bb-4824-b0f3-bae041d9d6a2
-  if (request.url.includes('session?delete=true')) {
+  if (request.url.includes('session?delete=true') || request.url.includes('session/heartbeat?requestId=')) {
+    // Offset for the query character preceding the 'requestId=' string in URL (either '?' or '&')
+    const PRECEDING_QUERY_CHAR_OFFSET = 1;
     // Remove the requestID query parameter for the mock HTTP client
-    request.url = request.url.substring(0, request.url.indexOf('&requestId='));
+    request.url = request.url.substring(0, request.url.indexOf('requestId=') - PRECEDING_QUERY_CHAR_OFFSET);
   }
 
   // get the output of the specified request from the map
@@ -85,9 +87,11 @@ MockHttpClient.prototype.requestAsync = function (request) {
 
   // Closing a connection includes a requestID as a query parameter in the url
   // Example: http://fake504.snowflakecomputing.com/session?delete=true&requestId=a40454c6-c3bb-4824-b0f3-bae041d9d6a2
-  if (request.url.includes('session?delete=true')) {
+  if (request.url.includes('session?delete=true') || request.url.includes('session/heartbeat?requestId=')) {
+    // Offset for the query character preceding the 'requestId=' string in URL (either '?' or '&')
+    const PRECEDING_QUERY_CHAR_OFFSET = 1;
     // Remove the requestID query parameter for the mock HTTP client
-    request.url = request.url.substring(0, request.url.indexOf('&requestId='));
+    request.url = request.url.substring(0, request.url.indexOf('requestId=') - PRECEDING_QUERY_CHAR_OFFSET);
   }
 
   // get the output of the specified request from the map
@@ -1997,6 +2001,35 @@ function buildRequestOutputMappings(clientInfo) {
                 }
             }
         }
+    },
+    {
+      request:
+        {
+          method: 'POST',
+          url: 'http://fakeaccount.snowflakecomputing.com/session/heartbeat',
+          headers:
+            {
+              'Accept': 'application/json',
+              'Authorization': 'Snowflake Token="SESSION_TOKEN"',
+              'Content-Type': 'application/json',
+            }
+        },
+      output:
+        {
+          err: null,
+          response:
+            {
+              statusCode: 200,
+              statusMessage: 'OK',
+              body:
+                {
+                  code: null,
+                  data: null,
+                  message: null,
+                  success: true
+                }
+            }
+        }
     }
   ];
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		exports.HEARTBEAT_FREQUENCY_MASTER_VALIDITY = 14400;