diff --git a/.github/scripts/run_ft_tests.bash b/.github/scripts/run_ft_tests.bash index bd6683b3..e080a860 100755 --- a/.github/scripts/run_ft_tests.bash +++ b/.github/scripts/run_ft_tests.bash @@ -17,6 +17,16 @@ if [ -z "$SETUP_CMD" ]; then SETUP_CMD="start" fi -UTAPI_INTERVAL_TEST_MODE=$1 npm $SETUP_CMD 2>&1 | tee -a "setup_$2.log" & +# Redirect stderr to a separate file (without "Killed" to look for warnings / error messages) +# While still keeping it in terminal +# ignore punycode warning from oas-tools@2.2.2 in utapiV2 tests + +UTAPI_INTERVAL_TEST_MODE=$1 npm $SETUP_CMD \ + 2> >(grep -v -E "^Killed$|--trace-deprecation|punycode" | tee -a "setup_$2.stderr.log" >&2) \ + | tee -a "setup_$2.log" & + bash tests/utils/wait_for_local_port.bash $PORT 40 -UTAPI_INTERVAL_TEST_MODE=$1 npm run $2 | tee -a "test_$2.log" + +UTAPI_INTERVAL_TEST_MODE=$1 npm run $2 \ + 2> >(tee -a "test_$2.stderr.log" >&2) \ + | tee -a "test_$2.log" diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 80bcbf3f..6a601e7b 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -18,6 +18,11 @@ on: required: false description: Timeout for ssh connection to worker (minutes) default: 30 + +env: + AWS_SDK_JS_SUPPRESS_MAINTENANCE_MODE_MESSAGE: '1' + # NODE_OPTIONS: '--trace-warnings' + jobs: build-ci: uses: ./.github/workflows/build-ci.yaml @@ -137,6 +142,11 @@ jobs: - name: ${{ matrix.test.name }} run: ${{ matrix.test.command }} env: ${{ matrix.test.env }} + - name: Print stderr logs + run: grep -H "" *.stderr.log || true + if: always() + - name: Error if stderr logs are not empty + run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi tests-v2-with-vault: needs: @@ -242,6 +252,13 @@ jobs: tmate-server-rsa-fingerprint: ${{ secrets.TMATE_SERVER_RSA_FINGERPRINT }} tmate-server-ed25519-fingerprint: ${{ secrets.TMATE_SERVER_ED25519_FINGERPRINT }} if: ${{ ( github.event.inputs.debug == true || github.event.inputs.debug == 'true' ) }} + - name: Print stderr logs + run: grep -H "" *.stderr.log || true + if: always() + - name: Error if stderr logs are not empty + run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi + + tests-v2-without-sensision: needs: - build-ci @@ -358,3 +375,8 @@ jobs: tmate-server-rsa-fingerprint: ${{ secrets.TMATE_SERVER_RSA_FINGERPRINT }} tmate-server-ed25519-fingerprint: ${{ secrets.TMATE_SERVER_ED25519_FINGERPRINT }} if: ${{ ( github.event.inputs.debug == true || github.event.inputs.debug == 'true' ) }} + - name: Print stderr logs + run: grep -H "" *.stderr.log || true + if: always() + - name: Error if stderr logs are not empty + run: if [ -n "$(find . -name '*.stderr.log' -size +0c)" ]; then exit 1; fi diff --git a/bin/ensureServiceUser b/bin/ensureServiceUser index d3737e30..a894ece5 100755 --- a/bin/ensureServiceUser +++ b/bin/ensureServiceUser @@ -4,6 +4,7 @@ // - deduplicate with Vault's seed script at https://github.com/scality/Vault/pull/1627 // - add permission boundaries to user when https://scality.atlassian.net/browse/VAULT-4 is implemented +process.env.AWS_SDK_JS_SUPPRESS_MAINTENANCE_MODE_MESSAGE = '1'; const { errors } = require('arsenal'); const { program } = require('commander'); const werelogs = require('werelogs'); diff --git a/libV2/process.js b/libV2/process.js index 0aa47b97..d0334271 100644 --- a/libV2/process.js +++ b/libV2/process.js @@ -31,6 +31,9 @@ class Process extends EventEmitter { async join() { this.emit('exit'); await this._join(); + ['SIGINT', 'SIGQUIT', 'SIGTERM', 'uncaughtException'].forEach(eventName => { + process.removeAllListeners(eventName); + }); } async _setup() {} diff --git a/libV2/redis.js b/libV2/redis.js index 671416c1..91ddeb45 100644 --- a/libV2/redis.js +++ b/libV2/redis.js @@ -1,5 +1,5 @@ const EventEmitter = require('events'); -const { callbackify, promisify } = require('util'); +const { callbackify } = require('util'); const IORedis = require('ioredis'); const { jsutil } = require('arsenal'); const BackOff = require('backo'); @@ -7,6 +7,7 @@ const { whilst } = require('async'); const errors = require('./errors'); const { LoggerContext, asyncOrCallback } = require('./utils'); +const flexiblePromisify = require('./utils/flexiblePromisify'); const moduleLogger = new LoggerContext({ module: 'redis', @@ -201,7 +202,9 @@ class RedisClient extends EventEmitter { if (callback !== undefined) { // If a callback is provided `func` is assumed to also take a callback // and is converted to a promise using promisify - return callbackify(this._call.bind(this))(promisify(func), callback); + // Note (DEP0174): flexiblePromisify avoids promisifying a function that already returns a promise + // With redisClientV2 func returns a promise even if there is a callback + return callbackify(this._call.bind(this))(flexiblePromisify(func), callback); } return this._call(func); } diff --git a/libV2/utils/flexiblePromisify.js b/libV2/utils/flexiblePromisify.js new file mode 100644 index 00000000..d815a4a3 --- /dev/null +++ b/libV2/utils/flexiblePromisify.js @@ -0,0 +1,42 @@ +/** + * Promisifies a function. If the function already returns a promise, + * it returns it as is. Handles both callbacks and promises. + * + * @param {Function} originalFn The function to promisify. + * @returns {Function} A function that returns a promise. + */ +module.exports = function flexiblePromisify(originalFn) { + // Check if the function provides a custom promise-based version. + // This is the same mechanism Node's util.promisify uses. + const customPromisified = originalFn[Symbol.for('nodejs.util.promisify.custom')]; + if (typeof customPromisified === 'function') { + return customPromisified; + } + + // Return the new promise-based wrapper function. + return function flexiblePromisifiedWrapper(...args) { + const thisCtx = this; + + return new Promise((resolve, reject) => { + // 1. Callback will be used if `originalFn` is a callback-style function. + function callback(err, result) { + if (err) { + return reject(err); + } + return resolve(result); + } + + // 2. Call the originalFn, with user's args AND our custom callback. + const potentialPromise = originalFn.apply(thisCtx, [...args, callback]); + + // 3. If originalFn returned a promise, we use its result and ignore the callback. + if (potentialPromise && typeof potentialPromise.then === 'function') { + // The function returned a promise. We'll trust it as the source of truth. + potentialPromise.then(resolve, reject); + } + + // If the function did NOT return a promise (i.e., it's a standard callback function), + // then our promise is already wired up to be resolved or rejected by the `callback` we passed in. + }); + }; +}; diff --git a/tests/unit/v2/utils/testFlexiblePromise.js b/tests/unit/v2/utils/testFlexiblePromise.js new file mode 100644 index 00000000..62b8e334 --- /dev/null +++ b/tests/unit/v2/utils/testFlexiblePromise.js @@ -0,0 +1,274 @@ +const assert = require('assert'); +const { scheduler } = require('timers/promises'); +const sinon = require('sinon'); +const flexiblePromisify = require('../../../../libV2/utils/flexiblePromisify'); + +describe('flexiblePromisify', () => { + describe('Custom promisified functions', () => { + it('should return custom promisified version when available', () => { + const customPromisified = () => Promise.resolve('custom result'); + const originalFn = () => {}; + originalFn[Symbol.for('nodejs.util.promisify.custom')] = customPromisified; + + const result = flexiblePromisify(originalFn); + + assert.strictEqual(result, customPromisified); + }); + + it('should use custom promisified function even if original has callback signature', () => { + const customPromisified = () => Promise.resolve('custom result'); + const originalFn = callback => callback(null, 'original result'); + originalFn[Symbol.for('nodejs.util.promisify.custom')] = customPromisified; + + const result = flexiblePromisify(originalFn); + + assert.strictEqual(result, customPromisified); + }); + }); + + describe('Promise-returning functions', () => { + it('should handle functions that return resolved promises', async () => { + const originalFn = () => Promise.resolve('promise result'); + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'promise result'); + }); + + it('should handle functions that return rejected promises', async () => { + const error = new Error('promise error'); + const originalFn = () => Promise.reject(error); + const promisified = flexiblePromisify(originalFn); + + await assert.rejects(promisified(), error); + }); + + it('should handle functions with arguments that return promises', async () => { + const originalFn = (a, b) => Promise.resolve(a + b); + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(5, 3); + + assert.strictEqual(result, 8); + }); + + it('should preserve this context for promise-returning functions', async () => { + const obj = { + value: 42, + fn() { + return Promise.resolve(this.value); + } + }; + const promisified = flexiblePromisify(obj.fn); + + const result = await promisified.call(obj); + + assert.strictEqual(result, 42); + }); + + it('should ignore asynchronous callback when function returns a promise', async () => { + const callbackSpy = sinon.spy(); + const originalFn = callback => { + // Even though we have a callback parameter, we return a promise + // The callback should be ignored + setTimeout(() => { + callbackSpy(); + callback(null, 'callback result'); + }, 10); + return Promise.resolve('promise result'); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'promise result'); + // Give callback time to potentially execute + await scheduler.wait(20); + assert.strictEqual(callbackSpy.callCount, 1); // Callback still executes but is ignored + }); + + it('should use synchronous callback result over returned promise', async () => { + const originalFn = callback => { + callback(null, 'callback result'); + return Promise.resolve('promise result'); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'callback result'); + }); + + it('should handle functions that return thenable objects', async () => { + const thenable = { + then: resolve => { + setTimeout(() => resolve('thenable result'), 10); + } + }; + const originalFn = () => thenable; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'thenable result'); + }); + + it('should handle functions that return rejected thenable objects', async () => { + const error = new Error('thenable error'); + const thenable = { + then: (resolve, reject) => { + setTimeout(() => reject(error), 10); + } + }; + const originalFn = () => thenable; + const promisified = flexiblePromisify(originalFn); + + await assert.rejects(promisified(), error); + }); + }); + + describe('Callback-style functions', () => { + it('should handle successful callback functions', async () => { + const originalFn = callback => { + setTimeout(() => callback(null, 'callback result'), 10); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'callback result'); + }); + + it('should handle callback functions with errors', async () => { + const error = new Error('callback error'); + const originalFn = callback => { + setTimeout(() => callback(error), 10); + }; + const promisified = flexiblePromisify(originalFn); + + await assert.rejects(promisified(), error); + }); + + it('should handle callback functions with arguments', async () => { + const originalFn = (a, b, callback) => { + setTimeout(() => callback(null, a * b), 10); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(4, 5); + + assert.strictEqual(result, 20); + }); + + it('should preserve this context for callback functions', async () => { + const obj = { + multiplier: 3, + fn(value, callback) { + setTimeout(() => callback(null, value * this.multiplier), 10); + } + }; + const promisified = flexiblePromisify(obj.fn); + + const result = await promisified.call(obj, 7); + + assert.strictEqual(result, 21); + }); + + it('should handle synchronous callback functions', async () => { + const originalFn = (value, callback) => { + callback(null, value.toUpperCase()); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified('hello'); + + assert.strictEqual(result, 'HELLO'); + }); + + it('should handle synchronous callback functions with errors', async () => { + const error = new Error('sync error'); + const originalFn = callback => { + callback(error); + }; + const promisified = flexiblePromisify(originalFn); + + await assert.rejects(promisified(), error); + }); + + it('should handle functions that return non-thenable objects', async () => { + const originalFn = callback => { + callback(null, 'success'); + return { notAPromise: true }; + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'success'); + }); + + it('should handle functions that throw synchronously', async () => { + const error = new Error('sync throw'); + const originalFn = () => { + throw error; + }; + const promisified = flexiblePromisify(originalFn); + + await assert.rejects(promisified(), error); + }); + + it('should handle callback functions that call callback multiple times', async () => { + const callbackSpy = sinon.spy(); + const originalFn = callback => { + callbackSpy(); + callback(null, 'first call'); + setTimeout(() => { + callbackSpy(); + callback(null, 'second call'); + }, 10); + }; + const promisified = flexiblePromisify(originalFn); + + const result = await promisified(); + + assert.strictEqual(result, 'first call'); + // Wait to see if second callback was called + await scheduler.wait(20); + assert.strictEqual(callbackSpy.callCount, 2); + }); + }); + + describe('Integration with Node.js util.promisify', () => { + it('should work similarly to util.promisify for callback functions', async () => { + const { promisify } = require('util'); + const originalFn = (value, callback) => { + setTimeout(() => callback(null, value * 2), 10); + }; + + const utilPromisified = promisify(originalFn); + const flexiblePromisified = flexiblePromisify(originalFn); + + const [utilResult, flexibleResult] = await Promise.all([ + utilPromisified(5), + flexiblePromisified(5) + ]); + + assert.strictEqual(utilResult, flexibleResult); + assert.strictEqual(utilResult, 10); + }); + + it('should respect custom promisify symbol like util.promisify', () => { + const { promisify } = require('util'); + const customPromisified = () => Promise.resolve('custom'); + const originalFn = () => {}; + originalFn[Symbol.for('nodejs.util.promisify.custom')] = customPromisified; + + const utilResult = promisify(originalFn); + const flexibleResult = flexiblePromisify(originalFn); + + assert.strictEqual(utilResult, customPromisified); + assert.strictEqual(flexibleResult, customPromisified); + }); + }); +});