Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
84 changes: 80 additions & 4 deletions famis_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import {
default as Axios,
AxiosError,
AxiosInstance,
AxiosRequestConfig,
AxiosResponse,
Method,
default as axios,
} from 'axios';
import * as AxiosLogger from 'axios-logger';
import axiosRetry from 'axios-retry';
import axiosRetry, { isNetworkOrIdempotentRequestError } from 'axios-retry';
import Bottleneck from 'bottleneck';
import _ from 'lodash';
import { ApiError, AuthorizationError } from './errors';
Expand Down Expand Up @@ -187,6 +188,22 @@ export const DefaultPropertyExpand = [
];
export const DefaultSpaceSelect = ['Id', 'Name', 'LongDescription'];

/**
* Default per-REQUEST timeout (ms). This bounds a single HTTP round-trip, NOT a whole
* paginated fetch: the client pages at $top=1000 and each page is its own request, so a
* multi-page (multi-minute/hour) fetch is unaffected — only an individual request that
* stalls (e.g. queued server-side under load, or hitting an unhealthy endpoint) trips it.
* Without this, a stalled request waits forever (axios default timeout = 0) and holds the
* caller's slot indefinitely. Generous by design; override per-client or per-call as needed.
*/
export const DEFAULT_REQUEST_TIMEOUT_MS = 120_000;

/**
* Default timeout (ms) for the login/token request specifically — always a small, quick
* call, so a much tighter bound is safe. A hung login is never legitimate.
*/
export const DEFAULT_LOGIN_TIMEOUT_MS = 30_000;

export class FamisClient {
host: string;
http: AxiosInstance;
Expand Down Expand Up @@ -218,12 +235,17 @@ export class FamisClient {
* here) if a request returns 401 AND the refresh-token attempt also fails. Defaults to false.
*/
reauthOnFailure?: boolean;
/** Per-request timeout (ms) for all subsequent calls. Defaults to DEFAULT_REQUEST_TIMEOUT_MS. */
requestTimeoutMs?: number;
/** Timeout (ms) for the initial login request. Defaults to DEFAULT_LOGIN_TIMEOUT_MS. */
loginTimeoutMs?: number;
}) {
const cred = await this.login({
username: opts.username,
password: opts.password,
url: opts.host,
debug: opts.debug,
timeoutMs: opts.loginTimeoutMs,
});
if (opts.debug === true) {
console.log(`Logged in with ${JSON.stringify(cred)}`);
Expand All @@ -238,6 +260,7 @@ export class FamisClient {
opts.reauthOnFailure ?? false,
// Only retain credentials when the caller explicitly opted in.
opts.reauthOnFailure ? { username: opts.username, password: opts.password } : undefined,
opts.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS,
);
}

Expand All @@ -247,6 +270,8 @@ export class FamisClient {
debug?: boolean;
autoRetry?: boolean;
onComplete?: OnCompleteCallback;
/** Per-request timeout (ms) for all calls. Defaults to DEFAULT_REQUEST_TIMEOUT_MS. */
requestTimeoutMs?: number;
}): FamisClient {
return new FamisClient(
{
Expand All @@ -267,6 +292,9 @@ export class FamisClient {
opts.debug ?? false,
opts.autoRetry ?? false,
opts.onComplete,
false,
undefined,
opts.requestTimeoutMs ?? DEFAULT_REQUEST_TIMEOUT_MS,
);
}

Expand All @@ -275,9 +303,12 @@ export class FamisClient {
password: string;
url: string;
debug?: boolean;
/** Timeout (ms) for the login request. Defaults to DEFAULT_LOGIN_TIMEOUT_MS. */
timeoutMs?: number;
}): Promise<LoginResponse> {
const http = axios.create({
baseURL: opts.url,
timeout: opts.timeoutMs ?? DEFAULT_LOGIN_TIMEOUT_MS,
});
if (opts?.debug === true)
console.log(`Logging into ${opts.url}. ${(opts.username, opts.password)}`);
Expand Down Expand Up @@ -333,6 +364,7 @@ export class FamisClient {
onComplete?: OnCompleteCallback,
reauthOnFailure: boolean = false,
reauthCredentials?: { username: string; password: string },
requestTimeoutMs: number = DEFAULT_REQUEST_TIMEOUT_MS,
) {
this.credentials = credentials;
this.host = host;
Expand All @@ -344,11 +376,25 @@ export class FamisClient {
baseURL: host,
validateStatus: (status) => true,
maxBodyLength: Infinity,
// Per-request timeout so a stalled request fails fast (and axios destroys the
// socket, freeing the slot) instead of hanging forever. Per-request, not
// per-operation — safe for paginated fetches (each page is its own request).
timeout: requestTimeoutMs,
});
if (autoRetry) {
axiosRetry(this.http, {
retries: 2,
retryDelay: () => 2,
// A per-request timeout (ECONNABORTED) fires on a stall; retrying it repeatedly just
// multiplies the wait. Allow at most ONE retry on timeout, while leaving the normal
// network/idempotent retry behavior (up to `retries`) intact for other failures.
retryCondition: (error: AxiosError) => {
if (error.code === 'ECONNABORTED') {
const retryCount = (error.config as any)?.['axios-retry']?.retryCount ?? 0;
return retryCount < 1;
}
return isNetworkOrIdempotentRequestError(error);
},
});
}

Expand Down Expand Up @@ -1787,14 +1833,43 @@ export class FamisClient {
};
}

/**
* Per-request axios config carrying a QueryContext's timeout override and/or AbortSignal,
* if set. Only includes keys that are set, so the client-level default `timeout` still
* applies when the context provides no override.
*
* axios 0.21 has no native `signal` support, so a caller-supplied AbortSignal is bridged
* to an axios CancelToken: aborting the signal cancels the in-flight request (and an
* already-aborted signal cancels immediately, before the request is dispatched).
*/
private requestConfig(context?: QueryContext): AxiosRequestConfig {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding #1 (medium) — requestConfig is not applied on the getAllUsingLink path. getAll<T> sends any supportsNextLink()==true type through getAllUsingLink(context.buildUrl(type)), which calls this.http.get(startPath) / this.http.get(nextLink) with no config. Since supportsNextLink defaults to true, that's the majority of the ~100 getAll-backed reads — so setSignal()/setTimeout() do nothing for them (only the 5 explicitly-false types hit getAllPaged). AbortSignal cancellation in particular is silently a no-op. Suggest passing the QueryContext into getAllUsingLink and applying this.requestConfig(context) to both gets, or caveating the setSignal/setTimeout JSDoc.

const config: AxiosRequestConfig = {};
if (context?.timeoutMs != null) {
config.timeout = context.timeoutMs;
}
if (context?.signal != null) {
const signal = context.signal;
const source = axios.CancelToken.source();
if (signal.aborted) {
source.cancel('Request aborted by caller');
} else {
signal.addEventListener('abort', () => source.cancel('Request aborted by caller'), {
once: true,
});
}
config.cancelToken = source.token;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding #3 (low, note only) — abort listener retained until signal GC. Each request builds a fresh CancelToken and registers a once:'abort' listener; on normal completion the listener isn't removed. For a signal reused across a multi-page getAllBatch (one context+signal, N pages) they accumulate until the signal is collected. Not a correctness bug (AbortSignal is EventTarget, no max-listeners warning) and per-operation signals are short-lived — just flagging.

}
return config;
}

async getAllBatch<T>(
context: QueryContext,
type: string,
callback: ResultCallback<T>,
): Promise<void> {
let top = 1000;
const url = context.buildPagedUrl(type, top, 0, true);
const resp = await this.http.get(url);
const resp = await this.http.get(url, this.requestConfig(context));

this.throwResponseError(resp);
const famisResp = resp.data as FamisResponse<T>;
Expand All @@ -1815,7 +1890,7 @@ export class FamisClient {
const url = context.buildPagedUrl(type, top, i * top);

const req = limiter
.schedule(() => this.http.get(url))
.schedule(() => this.http.get(url, this.requestConfig(context)))
.then((resp: AxiosResponse<FamisResponse<T>>) => {
this.throwResponseError(resp);
if (this.debug) {
Expand Down Expand Up @@ -1852,7 +1927,7 @@ export class FamisClient {
if (this.debug) {
console.log(`Fetching ${url}`);
}
const resp = await this.http.get(url);
const resp = await this.http.get(url, this.requestConfig(context));
durationMs += moment(Date.now()).diff(startDate);
this.throwResponseError(resp);
const famisResp = resp.data as FamisResponse<T>;
Expand Down Expand Up @@ -1900,6 +1975,7 @@ export class FamisClient {
const url = context.buildUrl('attachmentstream');
return await this.http.get(url, {
responseType: 'arraybuffer',
...this.requestConfig(context),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding #2 (low/medium) — the new 120s default now bounds attachment downloads. getAttachmentStream uses the client instance (timeout: 120_000) with responseType: 'arraybuffer'; axios 0.21 applies timeout across the full buffered-body download. A large attachment over a slow mobile link that previously completed (no timeout existed before) can now abort at 120s with ECONNABORTED. Consider a larger/disabled default for the attachment path, or document that callers should raise it via context.setTimeout().

});
}

Expand Down
1 change: 1 addition & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module.exports = {
'^.+\\.ts?$': 'ts-jest'
},
testEnvironment: 'node',
setupFiles: ['<rootDir>/tests/setup/abort-polyfill.js'],
testRegex: '/tests/.*\\.(test|spec)?\\.(ts|tsx|js)$',
moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'],
};
27 changes: 27 additions & 0 deletions model/request_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,19 @@ export class QueryContext {
top?: number;
skip?: number;
orderBy?: string;
/**
* Per-call request timeout override (ms) for the HTTP requests this context drives.
* Overrides the client's default `requestTimeoutMs`. Use for the rare endpoint whose
* single (non-paginated) response legitimately runs longer than the default. Paginated
* fetches don't need this — each page is a separate, individually-bounded request.
*/
timeoutMs?: number;
/**
* Per-call AbortSignal for the HTTP requests this context drives. Lets a caller cancel
* in-flight requests (e.g. a mobile user navigates away) instead of leaving them to run
* to the timeout. Never serialized into the OData URL.
*/
signal?: AbortSignal;

setFilter(filter: string | Filter) {
this.filter = filter.toString();
Expand Down Expand Up @@ -38,6 +51,18 @@ export class QueryContext {
return this;
}

/** Per-call HTTP request timeout override (ms). See `timeoutMs`. */
setTimeout(timeoutMs: number) {
this.timeoutMs = timeoutMs;
return this;
}

/** Per-call AbortSignal to cancel the HTTP requests this context drives. See `signal`. */
setSignal(signal: AbortSignal) {
this.signal = signal;
return this;
}

buildApiUrl(entity: string) {
let urlPath = `MobileWebServices/api/${entity}`;
return this.addFiltersToUrl(urlPath);
Expand Down Expand Up @@ -104,6 +129,8 @@ export class QueryContext {
this.filter = other.filter;
this.top = other.top;
this.skip = other.skip;
this.timeoutMs = other.timeoutMs;
this.signal = other.signal;
return this;
}
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "facility360",
"version": "1.1.4",
"version": "1.2.0",
"description": "A Node based 360Facility client SDK",
"main": "dist/index.js",
"scripts": {
Expand Down
29 changes: 29 additions & 0 deletions tests/query_context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,33 @@ describe('QueryContext', function () {
const uri = new URL(`http://example.com/${queryUrl}`);
expect(uri.searchParams.get('$filter')).toEqual("UserName eq 'O''Brien'");
});
it('carries a per-call timeout override via setTimeout (not part of the URL)', function () {
const context = new QueryContext().setSelect('Id').setTimeout(5000);
expect(context.timeoutMs).toEqual(5000);
// timeout is a request-config concern, never serialized into the OData URL
const uri = new URL(`http://example.com/${context.buildUrl('/test')}`);
expect(uri.searchParams.get('$select')).toEqual('Id');
expect(uri.href).not.toContain('timeout');
});
it('defaults timeoutMs to undefined so the client default applies', function () {
expect(new QueryContext().timeoutMs).toBeUndefined();
});
it('carries a per-call AbortSignal via setSignal (not part of the URL)', function () {
const controller = new AbortController();
const context = new QueryContext().setSelect('Id').setSignal(controller.signal);
expect(context.signal).toBe(controller.signal);
const uri = new URL(`http://example.com/${context.buildUrl('/test')}`);
expect(uri.searchParams.get('$select')).toEqual('Id');
expect(uri.href).not.toContain('signal');
});
it('defaults signal to undefined', function () {
expect(new QueryContext().signal).toBeUndefined();
});
it('copyFromOther carries timeoutMs and signal', function () {
const controller = new AbortController();
const source = new QueryContext().setTimeout(7000).setSignal(controller.signal);
const copy = new QueryContext().copyFromOther(source);
expect(copy.timeoutMs).toEqual(7000);
expect(copy.signal).toBe(controller.signal);
});
});
Loading
Loading