Skip to content

Commit

Permalink
Convert IPlatformHttp.doUri to use promises
Browse files Browse the repository at this point in the history 
The approach taken here is the same as that taken in 2001675, in order
to be able to emit both an error _and_ the response body.

Note that in the "just in case" handling of thrown errors I’ve just
typed the thrown error as `any` and not worried about the consequences
of doing so; we can figure out how to handle this properly and
consistently in #1617.
  • Loading branch information
@lawrence-forooghian
lawrence-forooghian committed Feb 8, 2024
1 parent fa5ea2e commit 1d9df7c
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 81 deletions.
2 changes: 1 addition & 1 deletion scripts/moduleReport.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { gzip } from 'zlib';
import Table from 'cli-table';

// The maximum size we allow for a minimal useful Realtime bundle (i.e. one that can subscribe to a channel)
const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 94, gzip: 29 };
const minimalUsefulRealtimeBundleSizeThresholdsKiB = { raw: 95, gzip: 29 };

const baseClientNames = ['BaseRest', 'BaseRealtime'];

Expand Down
6 changes: 5 additions & 1 deletion src/common/lib/util/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,10 @@ export function arrEquals(a: any[], b: any[]) {
);
}

export function throwMissingModuleError(moduleName: keyof ModulesMap): never {
export function createMissingModuleError(moduleName: keyof ModulesMap): ErrorInfo {
throw new ErrorInfo(`${moduleName} module not provided`, 40019, 400);
}

export function throwMissingModuleError(moduleName: keyof ModulesMap): never {
throw createMissingModuleError(moduleName);
}
29 changes: 24 additions & 5 deletions src/common/types/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@ export type RequestCallback = (
unpacked?: boolean,
statusCode?: number
) => void;

/**
* The `body`, `headers`, `unpacked`, and `statusCode` properties of a `RequestResult` may be populated even if its `error` property is non-null.
*/
export type RequestResult = {
error: RequestCallbackError | null;
body?: unknown;
headers?: RequestCallbackHeaders;
unpacked?: boolean;
statusCode?: number;
};

export type RequestParams = Record<string, string> | null;
export type RequestBody =
| Buffer // only on Node
Expand All @@ -34,18 +46,21 @@ export interface IPlatformHttp {
supportsAuthHeaders: boolean;
supportsLinkHeaders: boolean;

/**
* This method should not throw any errors; rather, it should communicate any error by populating the {@link RequestResult.error} property of the returned {@link RequestResult}.
*/
doUri(
method: HttpMethods,
uri: string,
headers: Record<string, string> | null,
body: RequestBody | null,
params: RequestParams,
callback?: RequestCallback
): void;
params: RequestParams
): Promise<RequestResult>;

checkConnectivity?: () => Promise<boolean>;

/**
* @param error An error returned by {@link doUri}’s callback.
* @param error An error from the {@link RequestResult.error} property of a result returned by {@link doUri}.
*/
shouldFallback(error: RequestCallbackError): boolean;
}
Expand Down Expand Up @@ -227,7 +242,11 @@ export class Http {
callback = logResponseHandler(callback, method, uri, params);
}

this.platformHttp.doUri(method, uri, headers, body, params, callback);
Utils.whenPromiseSettles(this.platformHttp.doUri(method, uri, 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)
);
}
}

Expand Down
71 changes: 31 additions & 40 deletions src/platform/nodejs/lib/util/http.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import {
ErrnoException,
RequestBody,
IPlatformHttpStatic,
RequestCallback,
RequestCallbackError,
RequestParams,
RequestResult,
} from '../../../../common/types/http';
import HttpMethods from '../../../../common/constants/HttpMethods';
import got, { Response, Options, CancelableRequest, Agents } from 'got';
Expand All @@ -16,7 +16,7 @@ import https from 'https';
import BaseClient from 'common/lib/client/baseclient';
import { RestAgentOptions } from 'common/types/ClientOptions';
import { isSuccessCode } from 'common/constants/HttpStatusCodes';
import { shallowEquals, throwMissingModuleError } from 'common/lib/util/utils';
import { createMissingModuleError, shallowEquals } from 'common/lib/util/utils';

/***************************************************
*
Expand All @@ -34,11 +34,10 @@ import { shallowEquals, throwMissingModuleError } from 'common/lib/util/utils';

const globalAgentPool: Array<{ options: RestAgentOptions; agents: Agents }> = [];

const handler = function (uri: string, params: unknown, client: BaseClient | null, callback?: RequestCallback) {
const handler = function (uri: string, params: unknown, client: BaseClient | null) {
return function (err: ErrnoException | null, response?: Response, body?: unknown) {
if (err) {
callback?.(err);
return;
return { error: err };
}
const statusCode = (response as Response).statusCode,
headers = (response as Response).headers;
Expand All @@ -49,7 +48,7 @@ const handler = function (uri: string, params: unknown, client: BaseClient | nul
break;
case 'application/x-msgpack':
if (!client?._MsgPack) {
throwMissingModuleError('MsgPack');
return { error: createMissingModuleError('MsgPack') };
}
body = client._MsgPack.decode(body as Buffer);
}
Expand All @@ -61,10 +60,9 @@ const handler = function (uri: string, params: unknown, client: BaseClient | nul
Number(headers['x-ably-errorcode']),
statusCode
);
callback?.(error, body, headers, true, statusCode);
return;
return { error, body, headers, unpacked: true, statusCode };
}
callback?.(null, body, headers, false, statusCode);
return { error: null, body, headers, unpacked: false, statusCode };
};
};

Expand All @@ -81,14 +79,13 @@ const Http: IPlatformHttpStatic = class {
this.client = client ?? null;
}

doUri(
async doUri(
method: HttpMethods,
uri: string,
headers: Record<string, string> | null,
body: RequestBody | null,
params: RequestParams,
callback: RequestCallback
): void {
params: RequestParams
): Promise<RequestResult> {
/* Will generally be making requests to one or two servers exclusively
* (Ably and perhaps an auth server), so for efficiency, use the
* foreverAgent to keep the TCP stream alive between requests where possible */
Expand Down Expand Up @@ -128,17 +125,15 @@ const Http: IPlatformHttpStatic = class {
// the same endpoint, inappropriately retrying 429s, etc
doOptions.retry = { limit: 0 };

(got[method](doOptions) as CancelableRequest<Response>)
.then((res: Response) => {
handler(uri, params, this.client, callback)(null, res, res.body);
})
.catch((err: ErrnoException) => {
if (err instanceof got.HTTPError) {
handler(uri, params, this.client, callback)(null, err.response, err.response.body);
return;
}
handler(uri, params, this.client, callback)(err);
});
try {
const res = await (got[method](doOptions) as CancelableRequest<Response>);
return handler(uri, params, this.client)(null, res, res.body);
} catch (err) {
if (err instanceof got.HTTPError) {
return handler(uri, params, this.client)(null, err.response, err.response.body);
}
return handler(uri, params, this.client)(err as ErrnoException);
}
}

checkConnectivity = async (): Promise<boolean> => {
Expand All @@ -149,22 +144,18 @@ const Http: IPlatformHttpStatic = class {
const connectivityCheckParams = this.client?.options.connectivityCheckParams ?? null;
const connectivityUrlIsDefault = !this.client?.options.connectivityCheckUrl;

return new Promise((resolve) => {
this.doUri(
HttpMethods.Get,
connectivityCheckUrl,
null,
null,
connectivityCheckParams,
function (err, responseText, headers, unpacked, statusCode) {
if (!err && !connectivityUrlIsDefault) {
resolve(isSuccessCode(statusCode as number));
return;
}
resolve(!err && (responseText as Buffer | string)?.toString().trim() === 'yes');
}
);
});
const { error, statusCode, body } = await this.doUri(
HttpMethods.Get,
connectivityCheckUrl,
null,
null,
connectivityCheckParams
);

if (!error && !connectivityUrlIsDefault) {
return isSuccessCode(statusCode as number);
}
return !error && (body as Buffer | string)?.toString().trim() === 'yes';
};

shouldFallback(err: RequestCallbackError) {
Expand Down
66 changes: 32 additions & 34 deletions src/platform/web/lib/http/http.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import Platform from 'common/platform';
import Defaults from 'common/lib/util/defaults';
import ErrorInfo, { PartialErrorInfo } from 'common/lib/types/errorinfo';
import { RequestBody, RequestCallback, RequestCallbackError, RequestParams } from 'common/types/http';
import { RequestBody, RequestCallback, RequestCallbackError, RequestParams, RequestResult } from 'common/types/http';
import HttpMethods from 'common/constants/HttpMethods';
import BaseClient from 'common/lib/client/baseclient';
import XHRStates from 'common/constants/XHRStates';
Expand Down Expand Up @@ -81,25 +81,24 @@ const Http = class {
'(XHRRequest)Http.checkConnectivity()',
'Sending; ' + connectivityCheckUrl
);
return new Promise((resolve) => {
this.doUri(
HttpMethods.Get,
connectivityCheckUrl,
null,
null,
connectivityCheckParams,
function (err, responseText, headers, unpacked, statusCode) {
let result = false;
if (!connectivityUrlIsDefault) {
result = !err && isSuccessCode(statusCode as number);
} else {
result = !err && (responseText as string)?.replace(/\n/, '') == 'yes';
}
Logger.logAction(Logger.LOG_MICRO, '(XHRRequest)Http.checkConnectivity()', 'Result: ' + result);
resolve(result);
}
);
});

const requestResult = await this.doUri(
HttpMethods.Get,
connectivityCheckUrl,
null,
null,
connectivityCheckParams
);

let result = false;
if (!connectivityUrlIsDefault) {
result = !requestResult.error && isSuccessCode(requestResult.statusCode as number);
} else {
result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
}

Logger.logAction(Logger.LOG_MICRO, '(XHRRequest)Http.checkConnectivity()', 'Result: ' + result);
return result;
};
}
} else if (Platform.Config.fetchSupported && fetchRequestImplementation) {
Expand All @@ -109,13 +108,10 @@ const Http = class {
};
this.checkConnectivity = async function () {
Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Sending; ' + connectivityCheckUrl);
return new Promise((resolve) => {
this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null, function (err, responseText) {
const result = !err && (responseText as string)?.replace(/\n/, '') == 'yes';
Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result);
resolve(result);
});
});
const requestResult = await this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null);
const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
Logger.logAction(Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result);
return result;
};
} else {
this.Request = (method, uri, headers, params, body, callback) => {
Expand All @@ -127,19 +123,21 @@ const Http = class {
}
}

doUri(
async doUri(
method: HttpMethods,
uri: string,
headers: Record<string, string> | null,
body: RequestBody | null,
params: RequestParams,
callback: RequestCallback
): void {
params: RequestParams
): Promise<RequestResult> {
if (!this.Request) {
callback(new PartialErrorInfo('Request invoked before assigned to', null, 500));
return;
return { error: new PartialErrorInfo('Request invoked before assigned to', null, 500) };
}
this.Request(method, uri, headers, params, body, callback);
return new Promise((resolve) => {
this.Request!(method, uri, headers, params, body, (error, resBody, resHeaders, unpacked, statusCode) =>
resolve({ error, body: resBody, headers: resHeaders, unpacked, statusCode })
);
});
}

private Request?: (
Expand Down

0 comments on commit 1d9df7c

Please sign in to comment.