Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add echo requester and fix timeouts #14

Merged
merged 4 commits into from
Nov 25, 2021
Merged

fix: add echo requester and fix timeouts #14

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4743e00
fi: add echo requester and fix timeouts
millotp Nov 24, 2021
6ff1b57
review'
millotp Nov 24, 2021
cf55b91
rename resp to responseUtils
millotp Nov 24, 2021
4423c9e
fix: simplify model with types and simplify api (#15)
millotp Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clients/algoliasearch-client-javascript/client-search/apis.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ import { SearchApi } from './searchApi';
export class searchClient extends SearchApi {}

export * from '../utils/errors';
export { EchoRequester } from '../utils/EchoRequester';

export const APIS = [SearchApi];
13 changes: 7 additions & 6 deletions clients/algoliasearch-client-javascript/client-search/searchApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import http from 'http';
import { shuffle } from '../utils/helpers';
import { Transporter } from '../utils/Transporter';
import { Headers, Host, Request, RequestOptions } from '../utils/types';
import { Requester } from '../utils/Requester';

import { BatchObject } from '../model/batchObject';
import { BatchResponse } from '../model/batchResponse';
Expand Down Expand Up @@ -32,7 +32,7 @@ export class SearchApi {

protected interceptors: Interceptor[] = [];

constructor(appId: string, apiKey: string) {
constructor(appId: string, apiKey: string, requester?: Requester) {
this.setApiKey(SearchApiApiKeys.appId, appId);
this.setApiKey(SearchApiApiKeys.apiKey, apiKey);
this.transporter = new Transporter({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not in the scope of this PR, but just wanted to make sure everyone's aware of this; I would recommend making the hosts (or the whole transporter) something you can pass to the constructor as well. Once Metis is released, the client would need to work with different hosts than the one we set by default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should prepare this codebase for Metis even if we don't have an ETA?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Does not cost anything, but just wondering)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to keep in mind for sure, otherwise we might have to rework a lot later on 🙂 Besides the hosts part, not a lot should change, though (perhaps the retry strategy but that is a problem for later 😄 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I'll include the host too

Expand All @@ -57,6 +57,7 @@ export class SearchApi {
read: 5,
write: 30,
},
requester,
});
}

Expand Down Expand Up @@ -143,7 +144,7 @@ export class SearchApi {

await interceptorPromise;

return this.transporter.retryableRequest(request, requestOptions);
return this.transporter.request(request, requestOptions);
}
/**
*
Expand Down Expand Up @@ -208,7 +209,7 @@ export class SearchApi {

await interceptorPromise;

return this.transporter.retryableRequest(request, requestOptions);
return this.transporter.request(request, requestOptions);
}
/**
* Add an object to the index, automatically assigning it an object ID
Expand Down Expand Up @@ -285,7 +286,7 @@ export class SearchApi {

await interceptorPromise;

return this.transporter.retryableRequest(request, requestOptions);
return this.transporter.request(request, requestOptions);
}
/**
*
Expand Down Expand Up @@ -363,6 +364,6 @@ export class SearchApi {

await interceptorPromise;

return this.transporter.retryableRequest(request, requestOptions);
return this.transporter.request(request, requestOptions);
}
}
12 changes: 12 additions & 0 deletions clients/algoliasearch-client-javascript/utils/EchoRequester.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import { EndRequest, Response } from './types';
import { Requester } from './Requester';

export class EchoRequester extends Requester {
async send(request: EndRequest): Promise<Response> {
return {
content: JSON.stringify(request),
isTimedOut: false,
status: 200,
};
}
}
84 changes: 84 additions & 0 deletions clients/algoliasearch-client-javascript/utils/HttpRequester.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import { EndRequest, Response } from './types';
import * as http from 'http';
import * as https from 'https';
import { Requester } from './Requester';

export class HttpRequester extends Requester {
private httpAgent: http.Agent;
private httpsAgent: https.Agent;

constructor() {
super();
this.httpAgent = new http.Agent({ keepAlive: true });
this.httpsAgent = new https.Agent({ keepAlive: true });
}

async send(request: EndRequest): Promise<Response> {
return new Promise((resolve) => {
const url = new URL(request.url);

const path = url.search === null ? url.pathname : `${url.pathname}?${url.search}`;

const options: https.RequestOptions = {
agent: url.protocol === 'https:' ? this.httpsAgent : this.httpAgent,
hostname: url.hostname,
path,
method: request.method,
headers: request.headers,
...(url.port !== undefined ? { port: url.port || '' } : {}),
};

const req = (url.protocol === 'https:' ? https : http).request(options, (response) => {
let contentBuffers: Buffer[] = [];

response.on('data', (chunk) => {
contentBuffers = contentBuffers.concat(chunk);
});

response.on('end', () => {
clearTimeout(connectTimeout);
clearTimeout(responseTimeout as NodeJS.Timeout);

resolve({
status: response.statusCode || 0,
content: Buffer.concat(contentBuffers).toString(),
isTimedOut: false,
});
});
});

const createTimeout = (timeout: number, content: string): NodeJS.Timeout => {
return setTimeout(() => {
req.destroy();

resolve({
status: 0,
content,
isTimedOut: true,
});
}, timeout * 1000);
};

const connectTimeout = createTimeout(request.connectTimeout, 'Connection timeout');

let responseTimeout: NodeJS.Timeout | undefined;

req.on('error', (error) => {
clearTimeout(connectTimeout);
clearTimeout(responseTimeout!);
resolve({ status: 0, content: error.message, isTimedOut: false });
});

req.once('response', () => {
clearTimeout(connectTimeout);
responseTimeout = createTimeout(request.responseTimeout, 'Socket timeout');
});

if (request.data !== undefined) {
req.write(request.data);
}

req.end();
});
}
}
82 changes: 2 additions & 80 deletions clients/algoliasearch-client-javascript/utils/Requester.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,82 +1,4 @@
import { EndRequest, Response } from './types';
import * as http from 'http';
import * as https from 'https';

export class Requester {
private httpAgent: http.Agent;
private httpsAgent: https.Agent;

constructor() {
this.httpAgent = new http.Agent({ keepAlive: true });
this.httpsAgent = new https.Agent({ keepAlive: true });
}

async send(request: EndRequest): Promise<Response> {
return new Promise((resolve) => {
const url = new URL(request.url);

const path = url.search === null ? url.pathname : `${url.pathname}?${url.search}`;

const options: https.RequestOptions = {
agent: url.protocol === 'https:' ? this.httpsAgent : this.httpAgent,
hostname: url.hostname,
path,
method: request.method,
headers: request.headers,
...(url.port !== undefined ? { port: url.port || '' } : {}),
};

const req = (url.protocol === 'https:' ? https : http).request(options, (response) => {
let contentBuffers: Buffer[] = [];

response.on('data', (chunk) => {
contentBuffers = contentBuffers.concat(chunk);
});

response.on('end', () => {
clearTimeout(connectTimeout);
clearTimeout(responseTimeout as NodeJS.Timeout);

resolve({
status: response.statusCode || 0,
content: Buffer.concat(contentBuffers).toString(),
isTimedOut: false,
});
});
});

const createTimeout = (timeout: number, content: string): NodeJS.Timeout => {
return setTimeout(() => {
req.destroy();

resolve({
status: 0,
content,
isTimedOut: true,
});
}, timeout * 1000);
};

const connectTimeout = createTimeout(request.connectTimeout, 'Connection timeout');

let responseTimeout: NodeJS.Timeout | undefined;

req.on('error', (error) => {
clearTimeout(connectTimeout);
clearTimeout(responseTimeout!);
resolve({ status: 0, content: error.message, isTimedOut: false });
});

req.once('response', () => {
clearTimeout(connectTimeout);
responseTimeout = createTimeout(request.responseTimeout, 'Socket timeout');
});

if (request.data !== undefined) {
req.write(request.data);
}

req.end();
});
}
export abstract class Requester {
abstract send(request: EndRequest): Promise<Response>;
}
17 changes: 17 additions & 0 deletions clients/algoliasearch-client-javascript/utils/Response.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { Response } from './types';

export function isNetworkError({ isTimedOut, status }: Omit<Response, 'content'>): boolean {
return !isTimedOut && ~~status === 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get the ~~status === 0 part of this check? Do we get a status of 0 back when there's a network error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's when there's no response at all

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied it from the existing client, I don't know why there is the ~~ but yes, here it indicates an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks for clarifying! The ~~ ensures we're always working with an integer (even if status is a string etc), without impacting performance too much.

StackOverflow

}

export function isRetryable({ isTimedOut, status }: Omit<Response, 'content'>): boolean {
return (
isTimedOut ||
isNetworkError({ isTimedOut, status }) ||
(~~(status / 100) !== 2 && ~~(status / 100) !== 4)
);
}

export function isSuccess({ status }: Pick<Response, 'status'>): boolean {
return ~~(status / 100) === 2;
}
Loading