fix: Reconnect after receiving goaway

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) {

test/.jshintrc

@@ -1,4 +1,5 @@
 {
+  "esversion": 11,
   "expr": true,
   "strict": false,
   "mocha": true,

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.

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;
+    });
+  });
+});