Convert Http.do to use promises

src/common/lib/client/auth.ts

@@ -588,13 +588,18 @@ class Auth {
         'Auth.requestToken().requestToken',
         'Sending POST to ' + path + '; Token params: ' + JSON.stringify(signedTokenParams)
       );
-      this.client.http.do(
-        HttpMethods.Post,
-        tokenUri,
-        requestHeaders,
-        JSON.stringify(signedTokenParams),
-        null,
-        tokenCb as RequestCallback
+      Utils.whenPromiseSettles(
+        this.client.http.do(HttpMethods.Post, tokenUri, requestHeaders, JSON.stringify(signedTokenParams), null),
+        (err: any, result) =>
+          err
+            ? (tokenCb as RequestCallback)(err) // doUri isn’t meant to throw an error, but handle any just in case
+            : (tokenCb as RequestCallback)(
+                result!.error,
+                result!.body,
+                result!.headers,
+                result!.unpacked,
+                result!.statusCode
+              )
       );
     };
 

src/common/lib/client/resource.ts

@@ -12,7 +12,6 @@ import {
   appendingParams as urlFromPathAndParams,
   paramString,
 } from 'common/types/http';
-import { ErrnoException } from '../../types/http';
 
 async function withAuthDetails<T>(
   client: BaseClient,
@@ -327,19 +326,7 @@ class Resource {
         );
       }
 
-      type HttpResult = {
-        error?: ErrnoException | IPartialErrorInfo | null;
-        body?: unknown;
-        headers?: RequestCallbackHeaders;
-        unpacked?: boolean;
-        statusCode?: number;
-      };
-
-      const httpResult = await new Promise<HttpResult>((resolve) => {
-        client.http.do(method, path, headers, body, params, function (error, body, headers, unpacked, statusCode) {
-          resolve({ error, body, headers, unpacked, statusCode });
-        });
-      });
+      const httpResult = await client.http.do(method, path, headers, body, params);
 
       if (httpResult.error && Auth.isTokenErr(httpResult.error as ErrorInfo)) {
         /* token has expired, so get a new one */

src/common/lib/client/rest.ts

@@ -68,30 +68,26 @@ export class Rest {
     const timeUri = (host: string) => {
       return this.client.baseUri(host) + '/time';
     };
-    return new Promise((resolve, reject) => {
-      this.client.http.do(
-        HttpMethods.Get,
-        timeUri,
-        headers,
-        null,
-        params as RequestParams,
-        (err, body, headers, unpacked) => {
-          if (err) {
-            reject(err);
-            return;
-          }
-          if (!unpacked) body = JSON.parse(body as string);
-          const time = (body as number[])[0];
-          if (!time) {
-            reject(new ErrorInfo('Internal error (unexpected result type from GET /time)', 50000, 500));
-            return;
-          }
-          /* calculate time offset only once for this device by adding to the prototype */
-          this.client.serverTimeOffset = time - Utils.now();
-          resolve(time);
-        }
-      );
-    });
+
+    let { error, body, unpacked } = await this.client.http.do(
+      HttpMethods.Get,
+      timeUri,
+      headers,
+      null,
+      params as RequestParams
+    );
+
+    if (error) {
+      throw error;
+    }
+    if (!unpacked) body = JSON.parse(body as string);
+    const time = (body as number[])[0];
+    if (!time) {
+      throw new ErrorInfo('Internal error (unexpected result type from GET /time)', 50000, 500);
+    }
+    /* calculate time offset only once for this device by adding to the prototype */
+    this.client.serverTimeOffset = time - Utils.now();
+    return time;
   }
 
   async request(

src/common/types/http.ts

@@ -151,99 +151,74 @@ export class Http {
     return Defaults.getHosts(client.options);
   }
 
-  do(
+  /**
+   * This method will not throw any errors; rather, it will communicate any error by populating the {@link RequestResult.error} property of the returned {@link RequestResult}.
+   */
+  async do(
     method: HttpMethods,
     path: PathParameter,
     headers: Record<string, string> | null,
     body: RequestBody | null,
-    params: RequestParams,
-    callback?: RequestCallback | undefined
-  ): void {
-    /* Unlike for doUri, the presence of `this.client` here is mandatory, as it's used to generate the hosts */
-    const client = this.client;
-    if (!client) {
-      throw new Error('http.do called without client');
-    }
-
-    const uriFromHost =
-      typeof path === 'function'
-        ? path
-        : function (host: string) {
-            return client.baseUri(host) + path;
-          };
+    params: RequestParams
+  ): Promise<RequestResult> {
+    try {
+      /* Unlike for doUri, the presence of `this.client` here is mandatory, as it's used to generate the hosts */
+      const client = this.client;
+      if (!client) {
+        return { error: new ErrorInfo('http.do called without client', 50000, 500) };
+      }
 
-    const currentFallback = client._currentFallback;
-    if (currentFallback) {
-      if (currentFallback.validUntil > Date.now()) {
-        /* Use stored fallback */
-        Utils.whenPromiseSettles(
-          this.doUri(method, uriFromHost(currentFallback.host), headers, body, params),
-          (err: any, result) => {
-            // doUri isn’t meant to throw an error, but handle any just in case
-            if (err) {
-              callback?.(err);
-              return;
-            }
+      const uriFromHost =
+        typeof path === 'function'
+          ? path
+          : function (host: string) {
+              return client.baseUri(host) + path;
+            };
 
-            if (result!.error && this.platformHttp.shouldFallback(result!.error as ErrnoException)) {
-              /* unstore the fallback and start from the top with the default sequence */
-              client._currentFallback = null;
-              this.do(method, path, headers, body, params, callback);
-              return;
-            }
-            callback?.(result!.error, result!.body, result!.headers, result!.unpacked, result!.statusCode);
+      const currentFallback = client._currentFallback;
+      if (currentFallback) {
+        if (currentFallback.validUntil > Date.now()) {
+          /* Use stored fallback */
+          const result = await this.doUri(method, uriFromHost(currentFallback.host), headers, body, params);
+          if (result.error && this.platformHttp.shouldFallback(result.error as ErrnoException)) {
+            /* unstore the fallback and start from the top with the default sequence */
+            client._currentFallback = null;
+            return this.do(method, path, headers, body, params);
           }
-        );
-        return;
-      } else {
-        /* Fallback expired; remove it and fallthrough to normal sequence */
-        client._currentFallback = null;
+          return result;
+        } else {
+          /* Fallback expired; remove it and fallthrough to normal sequence */
+          client._currentFallback = null;
+        }
       }
-    }
 
-    const hosts = this._getHosts(client);
+      const hosts = this._getHosts(client);
 
-    /* see if we have one or more than one host */
-    if (hosts.length === 1) {
-      Utils.whenPromiseSettles(this.doUri(method, uriFromHost(hosts[0]), headers, body, params), (err: any, result) =>
-        err
-          ? callback?.(err) // doUri isn’t meant to throw an error, but handle any just in case
-          : callback?.(result!.error, result!.body, result!.headers, result!.unpacked, result!.statusCode)
-      );
-      return;
-    }
-
-    const tryAHost = (candidateHosts: Array<string>, persistOnSuccess?: boolean) => {
-      const host = candidateHosts.shift();
-      Utils.whenPromiseSettles(
-        this.doUri(method, uriFromHost(host as string), headers, body, params),
-        (err: any, result) => {
-          // doUri isn’t meant to throw an error, but handle any just in case
-          if (err) {
-            callback?.(err);
-            return;
-          }
+      /* see if we have one or more than one host */
+      if (hosts.length === 1) {
+        return this.doUri(method, uriFromHost(hosts[0]), headers, body, params);
+      }
 
-          if (
-            result!.error &&
-            this.platformHttp.shouldFallback(result!.error as ErrnoException) &&
-            candidateHosts.length
-          ) {
-            tryAHost(candidateHosts, true);
-            return;
-          }
-          if (persistOnSuccess) {
-            /* RSC15f */
-            client._currentFallback = {
-              host: host as string,
-              validUntil: Date.now() + client.options.timeouts.fallbackRetryTimeout,
-            };
-          }
-          callback?.(result!.error, result!.body, result!.headers, result!.unpacked, result!.statusCode);
+      const tryAHost = async (candidateHosts: Array<string>, persistOnSuccess?: boolean): Promise<RequestResult> => {
+        const host = candidateHosts.shift();
+        const result = await this.doUri(method, uriFromHost(host as string), headers, body, params);
+        if (result.error && this.platformHttp.shouldFallback(result.error as ErrnoException) && candidateHosts.length) {
+          return tryAHost(candidateHosts, true);
+        }
+        if (persistOnSuccess) {
+          /* RSC15f */
+          client._currentFallback = {
+            host: host as string,
+            validUntil: Date.now() + client.options.timeouts.fallbackRetryTimeout,
+          };
         }
-      );
-    };
-    tryAHost(hosts);
+        return result;
+      };
+      return tryAHost(hosts);
+    } catch (err) {
+      // Handle any unexpected error, to ensure we always meet our contract of not throwing any errors
+      return { error: new ErrorInfo(`Unexpected error in Http.do: ${Utils.inspectError(err)}`, 500, 50000) };
+    }
   }
 
   /**

test/browser/modules.test.js

@@ -394,12 +394,12 @@ function registerAblyModulesTests(helper, registerDeltaTests) {
         const channelName = 'channel';
         const channel = rest.channels.get(channelName);
         const contentTypeUsedForPublishPromise = new Promise((resolve, reject) => {
-          rest.http.do = (method, path, headers, body, params, callback) => {
+          rest.http.do = async (method, path, headers, body, params) => {
             if (!(method == 'post' && path == `/channels/${channelName}/messages`)) {
-              return;
+              return new Promise(() => {});
             }
             resolve(headers['content-type']);
-            callback(null);
+            return { error: null };
           };
         });
 

test/rest/http.test.js

@@ -25,7 +25,7 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) {
       var originalDo = rest.http.do;
 
       // Intercept Http.do with test
-      function testRequestHandler(method, path, headers, body, params, callback) {
+      async function testRequestHandler(method, path, headers, body, params) {
         expect('X-Ably-Version' in headers, 'Verify version header exists').to.be.ok;
         expect('Ably-Agent' in headers, 'Verify agent header exists').to.be.ok;
 
@@ -47,7 +47,7 @@ define(['ably', 'shared_helper', 'chai'], function (Ably, helper, chai) {
           expect(headers['Ably-Agent'].indexOf('nodejs') > -1, 'Verify agent').to.be.ok;
         }
 
-        originalDo.call(rest.http, method, path, headers, body, params, callback);
+        return originalDo.call(rest.http, method, path, headers, body, params);
       }
 
       rest.http.do = testRequestHandler;

test/rest/request.test.js

@@ -25,14 +25,15 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
     restTestOnJsonMsgpack('request_version', function (rest) {
       const version = 150; // arbitrarily chosen
 
-      function testRequestHandler(_, __, headers) {
+      async function testRequestHandler(_, __, headers) {
         try {
           expect('X-Ably-Version' in headers, 'Verify version header exists').to.be.ok;
           expect(headers['X-Ably-Version']).to.equal(version.toString(), 'Verify version number sent in request');
           done();
         } catch (err) {
           done(err);
         }
+        return new Promise(() => {});
       }
 
       rest.http.do = testRequestHandler;