diff --git a/lib/client.js b/lib/client.js index 8703e9b1..26a06b8a 100644 --- a/lib/client.js +++ b/lib/client.js @@ -39,7 +39,7 @@ module.exports = function (dependencies) { this.logger = defaultLogger; this.errorLogger = defaultErrorLogger; this.healthCheckInterval = setInterval(() => { - if (this.session && !this.session.closed && !this.session.destroyed && !this.isDestroyed) { + if (this.isStandardSessionConnected() && !this.isDestroyed) { this.session.ping((error, duration) => { if (error && this.errorLogger.enabled) { this.errorLogger( @@ -52,12 +52,7 @@ module.exports = function (dependencies) { } }, this.config.heartBeat).unref(); this.manageChannelsHealthCheckInterval = setInterval(() => { - if ( - this.manageChannelsSession && - !this.manageChannelsSession.closed && - !this.manageChannelsSession.destroyed && - !this.isDestroyed - ) { + if (this.isManagedChannelsSessionConnected() && !this.isDestroyed) { this.manageChannelsSession.ping((error, duration) => { if (error && this.errorLogger.enabled) { this.errorLogger( @@ -91,11 +86,13 @@ module.exports = function (dependencies) { return; } if (!session.closed) { + session._isClosing = true; await new Promise(resolve => { session.close(() => { resolve(); }); }); + session._isClosing = false; } this.destroySession(session); }; @@ -138,8 +135,53 @@ module.exports = function (dependencies) { return subDirectoryObject; }; + /** + * Determines if a request should be retried based on the error. This includes certain status codes, expired provider token, and transient write failures. + * @param {Object} error - An object representing the error which may include the following properties: + * @param {number} [error.status] - The HTTP status code returned from the APNs server. + * @param {VError} error.error - The error details which may include a message describing the error. + * @param {string} [error.error.message] - The error message which may indicate specific conditions such as 'ExpiredProviderToken' or transient write failures. + * @returns {boolean} - Returns true if the request is considered retryable based on the error, otherwise false. + */ + Client.isRequestRetryable = function isRequestRetryable(error) { + const isStatusCodeRetryable = [408, 429, 500, 502, 503, 504].includes(error.status); + + const isProviderTokenExpired = + error.status === 403 && error.error?.message === 'ExpiredProviderToken'; + + // This can happen after the server initiates a goaway, and the client did not finish closing and destroying the session yet, so the client tries to send a request on a closing session. + const isTransientWriteFailure = !!error.error?.message?.startsWith('apn write failed'); + + return isStatusCodeRetryable || isProviderTokenExpired || isTransientWriteFailure; + }; + + /** + * returns true if the standard session is connected and ready for use, false otherwise + * Note that even if this returns true, a request can still fail due to transient network issues, in which case the request will be retried according to the retry logic in the write method. + * @returns {boolean} + */ + Client.prototype.isStandardSessionConnected = function isStandardSessionConnected() { + return ( + this.session && !this.session._isClosing && !this.session.closed && !this.session.destroyed + ); + }; + + /** + * returns true if the manageChannels session is connected and ready for use, false otherwise + * Note that even if this returns true, a request can still fail due to transient network issues, in which case the request will be retried according to the retry logic in the write method. + * @returns {boolean} + */ + Client.prototype.isManagedChannelsSessionConnected = + function isManagedChannelsSessionConnected() { + return ( + this.manageChannelsSession && + !this.manageChannelsSession._isClosing && + !this.manageChannelsSession.closed && + !this.manageChannelsSession.destroyed + ); + }; + Client.prototype.write = async function write(notification, subDirectory, type, method, count) { - const retryStatusCodes = [408, 429, 500, 502, 503, 504]; const retryCount = count || 0; const subDirectoryLabel = this.subDirectoryLabel(type) ?? type; const subDirectoryInformation = this.makeSubDirectoryTypeObject( @@ -171,11 +213,7 @@ module.exports = function (dependencies) { if (path.includes('/1/apps/')) { // Connect manageChannelsSession. - if ( - !this.manageChannelsSession || - this.manageChannelsSession.closed || - this.manageChannelsSession.destroyed - ) { + if (!this.isManagedChannelsSessionConnected()) { try { await this.manageChannelsConnect(); } catch (error) { @@ -198,21 +236,14 @@ module.exports = function (dependencies) { ); return { ...subDirectoryInformation, ...sentRequest }; } catch (error) { - // Determine if this is a retryable request. - if ( - retryStatusCodes.includes(error.status) || - (typeof error.error !== 'undefined' && - error.status == 403 && - error.error.message === 'ExpiredProviderToken') - ) { + if (Client.isRequestRetryable(error)) { try { - const resentRequest = await this.retryRequest( + const resentRequest = await this.retryWrite( error, - this.manageChannelsSession, - this.config.manageChannelsAddress, notification, - path, - httpMethod, + subDirectory, + type, + method, retryCount ); return { ...subDirectoryInformation, ...resentRequest }; @@ -231,7 +262,7 @@ module.exports = function (dependencies) { } } else { // Connect to standard session. - if (!this.session || this.session.closed || this.session.destroyed) { + if (!this.isStandardSessionConnected()) { try { await this.connect(); } catch (error) { @@ -255,21 +286,14 @@ module.exports = function (dependencies) { ); return { ...subDirectoryInformation, ...sentRequest }; } catch (error) { - // Determine if this is a retryable request. - if ( - retryStatusCodes.includes(error.status) || - (typeof error.error !== 'undefined' && - error.status == 403 && - error.error.message === 'ExpiredProviderToken') - ) { + if (Client.isRequestRetryable(error)) { try { - const resentRequest = await this.retryRequest( + const resentRequest = await this.retryWrite( error, - this.session, - this.config.address, notification, - path, - httpMethod, + subDirectory, + type, + method, retryCount ); return { ...subDirectoryInformation, ...resentRequest }; @@ -289,55 +313,24 @@ module.exports = function (dependencies) { } }; - Client.prototype.retryRequest = async function retryRequest( + Client.prototype.retryWrite = async function retryWrite( error, - session, - address, notification, - path, - httpMethod, - count + subDirectory, + type, + method, + retryCount ) { - if (this.isDestroyed || session.closed) { - const error = { error: new VError('client session is either closed or destroyed') }; - throw error; - } - - const retryCount = count + 1; - - if (retryCount > this.config.connectionRetryLimit) { + if (retryCount >= this.config.connectionRetryLimit) { throw error; } const delayInSeconds = parseInt(error.retryAfter || 0); - // Obey servers request to try after a specific time in ms. const delayPromise = new Promise(resolve => setTimeout(resolve, delayInSeconds * 1000)); await delayPromise; - try { - const sentRequest = await this.request( - session, - address, - notification, - path, - httpMethod, - retryCount - ); - return sentRequest; - } catch (error) { - // Recursivelly call self until retryCount is exhausted - // or error is thrown. - const sentRequest = await this.retryRequest( - error, - session, - address, - notification, - path, - httpMethod, - retryCount - ); - return sentRequest; - } + // Retry write, which will handle reconnection if needed + return await this.write(notification, subDirectory, type, method, retryCount + 1); }; Client.prototype.connect = function connect() { @@ -371,11 +364,11 @@ module.exports = function (dependencies) { this.config )); - if (this.logger.enabled) { - this.session.on('connect', () => { + this.session.on('connect', () => { + if (this.logger.enabled) { this.logger('Session connected'); - }); - } + } + }); this.session.on('close', () => { if (this.errorLogger.enabled) { @@ -450,11 +443,11 @@ module.exports = function (dependencies) { config )); - if (this.logger.enabled) { - this.manageChannelsSession.on('connect', () => { + this.manageChannelsSession.on('connect', () => { + if (this.logger.enabled) { this.logger('ManageChannelsSession connected'); - }); - } + } + }); this.manageChannelsSession.on('close', () => { if (this.errorLogger.enabled) { diff --git a/test/.jshintrc b/test/.jshintrc index 01ad0ba5..c8769a60 100644 --- a/test/.jshintrc +++ b/test/.jshintrc @@ -1,4 +1,5 @@ { + "esversion": 11, "expr": true, "strict": false, "mocha": true, diff --git a/test/client.js b/test/client.js index 06902ca1..3f9988fd 100644 --- a/test/client.js +++ b/test/client.js @@ -665,7 +665,7 @@ describe('Client', () => { ]); }); - it('Handles goaway frames', async () => { + it('Handles goaway frames and retries the request on a new connection', async () => { let didGetRequest = false; let establishedConnections = 0; server = createAndStartMockLowLevelServer(TEST_PORT, stream => { @@ -714,7 +714,7 @@ describe('Client', () => { }; await performRequestExpectingGoAway(); await performRequestExpectingGoAway(); - expect(establishedConnections).to.equal(2); + expect(establishedConnections).to.equal(8); expect(errorMessages).to.not.be.empty; let errorMessagesContainsGoAway = false; // Search for message, in older node, may be in random order. @@ -2369,7 +2369,7 @@ describe('ManageChannelsClient', () => { expect(infoMessagesContainsTimeout).to.be.true; }); - it('Handles goaway frames', async () => { + it('Handles goaway frames and retries the request on a new connection', async () => { let didGetRequest = false; let establishedConnections = 0; server = createAndStartMockLowLevelServer(TEST_PORT, stream => { @@ -2418,7 +2418,7 @@ describe('ManageChannelsClient', () => { }; await performRequestExpectingGoAway(); await performRequestExpectingGoAway(); - expect(establishedConnections).to.equal(2); + expect(establishedConnections).to.equal(8); expect(errorMessages).to.not.be.empty; let errorMessagesContainsGoAway = false; // Search for message, in older node, may be in random order. diff --git a/test/isRequestRetryable.unit.js b/test/isRequestRetryable.unit.js new file mode 100644 index 00000000..46b5b460 --- /dev/null +++ b/test/isRequestRetryable.unit.js @@ -0,0 +1,61 @@ +const { expect } = require('chai'); +const Client = require('../lib/client')({ + logger: { enabled: false, log: () => {} }, + errorLogger: { enabled: false, log: () => {} }, + config: () => ({}), + http2: { constants: {} }, +}); + +describe('Client.isRequestRetryable', () => { + it('returns true for error.message starting with "apn write failed"', () => { + const error = { error: { message: 'apn write failed: socket hang up' } }; + expect(Client.isRequestRetryable(error)).to.be.true; + }); + + it('returns false for error.message starting with "apn write timeout" (not retryable)', () => { + const error = { error: { message: 'apn write timeout: timed out' } }; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns false for error.message starting with "apn write aborted" (not retryable)', () => { + const error = { error: { message: 'apn write aborted: aborted' } }; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns false for error.message with other apn messages', () => { + const error = { error: { message: 'apn connection closed' } }; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns false for null error.error', () => { + const error = { error: null }; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns false for undefined error.error', () => { + const error = {}; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns false for error.error.message not matching', () => { + const error = { error: { message: 'some other error' } }; + expect(Client.isRequestRetryable(error)).to.be.false; + }); + + it('returns true for retryable status codes', () => { + [408, 429, 500, 502, 503, 504].forEach(status => { + expect(Client.isRequestRetryable({ status })).to.be.true; + }); + }); + + it('returns true for ExpiredProviderToken', () => { + const error = { status: 403, error: { message: 'ExpiredProviderToken' } }; + expect(Client.isRequestRetryable(error)).to.be.true; + }); + + it('returns false for non-retryable status codes', () => { + [200, 201, 400, 404, 401, 403].forEach(status => { + expect(Client.isRequestRetryable({ status })).to.be.false; + }); + }); +}); diff --git a/test/multiclient.js b/test/multiclient.js index 80d46a18..9d6055ca 100644 --- a/test/multiclient.js +++ b/test/multiclient.js @@ -463,7 +463,7 @@ describe('MultiClient', () => { ]); }); - it('Handles goaway frames', async () => { + it('Handles goaway frames and retries the request on a new connection', async () => { let didGetRequest = false; let establishedConnections = 0; server = createAndStartMockLowLevelServer(TEST_PORT, stream => { @@ -499,7 +499,7 @@ describe('MultiClient', () => { }; await performRequestExpectingGoAway(); await performRequestExpectingGoAway(); - expect(establishedConnections).to.equal(2); + expect(establishedConnections).to.equal(8); }); it('Handles unexpected protocol errors (no response sent)', async () => { @@ -1564,7 +1564,7 @@ describe('ManageChannelsMultiClient', () => { ]); }); - it('Handles goaway frames', async () => { + it('Handles goaway frames and retries the request on a new connection', async () => { let didGetRequest = false; let establishedConnections = 0; server = createAndStartMockLowLevelServer(TEST_PORT, stream => { @@ -1600,7 +1600,7 @@ describe('ManageChannelsMultiClient', () => { }; await performRequestExpectingGoAway(); await performRequestExpectingGoAway(); - expect(establishedConnections).to.equal(2); + expect(establishedConnections).to.equal(8); }); it('Handles unexpected protocol errors (no response sent)', async () => {