-
Notifications
You must be signed in to change notification settings - Fork 22
TON-1531: abort controllers in api client #479
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
9f7d623
1a2d760
8c17d40
0bcfd94
185bb48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /** | ||
| * Copyright (c) TonTech. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| */ | ||
|
|
||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { BaseApiClient } from './BaseApiClient'; | ||
| import type { BaseApiClientConfig } from './BaseApiClient'; | ||
| import { ApiClientTimeoutError } from './errors'; | ||
|
|
||
| class TestClient extends BaseApiClient { | ||
| constructor(config: BaseApiClientConfig) { | ||
| super(config, 'https://example.test'); | ||
| } | ||
| protected appendAuthHeaders(): void {} | ||
| } | ||
|
|
||
| /** Resolves with a JSON response. */ | ||
| const jsonFetch = (body: unknown, status = 200): typeof fetch => | ||
| (async () => | ||
| new Response(JSON.stringify(body), { | ||
| status, | ||
| headers: { 'content-type': 'application/json' }, | ||
| })) as typeof fetch; | ||
|
|
||
| /** Never resolves on its own — only rejects (with the abort reason) once its signal aborts. */ | ||
| const hangingFetch = (): typeof fetch => | ||
| ((_input: RequestInfo | URL, init?: RequestInit) => | ||
| new Promise<Response>((_resolve, reject) => { | ||
| const signal = init?.signal; | ||
| signal?.addEventListener( | ||
| 'abort', | ||
| () => reject(signal.reason ?? new DOMException('Aborted', 'AbortError')), | ||
| { | ||
| once: true, | ||
| }, | ||
| ); | ||
| })) as typeof fetch; | ||
|
|
||
| describe('BaseApiClient', () => { | ||
| it('returns parsed JSON on success', async () => { | ||
| const client = new TestClient({ fetchApi: jsonFetch({ ok: 1 }), timeout: 1000 }); | ||
| await expect(client.getJson('/x')).resolves.toEqual({ ok: 1 }); | ||
| }); | ||
|
|
||
| it('throws ApiClientTimeoutError when the request exceeds the timeout', async () => { | ||
| const client = new TestClient({ fetchApi: hangingFetch(), timeout: 10 }); | ||
| await expect(client.getJson('/x')).rejects.toBeInstanceOf(ApiClientTimeoutError); | ||
| }); | ||
|
|
||
| it('propagates a caller abort without converting it to a timeout', async () => { | ||
| const controller = new AbortController(); | ||
| const client = new TestClient({ fetchApi: hangingFetch(), timeout: 1000 }); | ||
| const promise = client.getJson('/x', undefined, { signal: controller.signal }); | ||
| controller.abort(); | ||
| await expect(promise).rejects.not.toBeInstanceOf(ApiClientTimeoutError); | ||
| }); | ||
|
|
||
| it('aborts before touching the network when the signal is already aborted', async () => { | ||
| const fetchApi = vi.fn(hangingFetch()); | ||
| const client = new TestClient({ fetchApi, timeout: 1000 }); | ||
| await expect(client.getJson('/x', undefined, { signal: AbortSignal.abort() })).rejects.toThrow(); | ||
| expect(fetchApi).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,9 @@ | |
| */ | ||
|
|
||
| import { Network } from '../api/models'; | ||
| import { TonClientError } from './TonClientError'; | ||
| import { combineSignals } from './combine-signals'; | ||
| import { ApiClientHttpError, ApiClientTimeoutError } from './errors'; | ||
| import type { RequestOptions } from './types'; | ||
|
|
||
| export interface BaseApiClientConfig { | ||
| endpoint?: string; | ||
|
|
@@ -37,34 +39,37 @@ export abstract class BaseApiClient { | |
|
|
||
| protected abstract appendAuthHeaders(headers: Headers): void; | ||
|
|
||
| async fetch<T>(url: URL, props: globalThis.RequestInit = {}): Promise<T> { | ||
| async fetch<T>(url: URL, props: globalThis.RequestInit = {}, opts: RequestOptions = {}): Promise<T> { | ||
| const headers = new Headers(props.headers); | ||
| headers.set('accept', 'application/json'); | ||
| this.appendAuthHeaders(headers); | ||
| props = { ...props, headers }; | ||
| const response = await this.doRequest(url, props); | ||
| const response = await this.doRequest(url, { ...props, headers }, opts.signal); | ||
| if (!response.ok) { | ||
| throw await this.buildError(response); | ||
| } | ||
| const contentType = response.headers.get('content-type') || ''; | ||
| if (!contentType.includes('application/json')) { | ||
| const text = await (response as globalThis.Response).text(); | ||
| throw new TonClientError('Unexpected non-JSON response', response.status, text.slice(0, 200)); | ||
| throw new ApiClientHttpError('Unexpected non-JSON response', response.status, text.slice(0, 200)); | ||
| } | ||
| const json = await response.json(); | ||
| return json as Promise<T>; | ||
| } | ||
|
|
||
| async getJson<T>(path: string, query?: Record<string, unknown>): Promise<T> { | ||
| return this.fetch(this.buildUrl(path, query), { method: 'GET' }); | ||
| async getJson<T>(path: string, query?: Record<string, unknown>, opts?: RequestOptions): Promise<T> { | ||
| return this.fetch(this.buildUrl(path, query), { method: 'GET' }, opts); | ||
| } | ||
|
|
||
| async postJson<T>(path: string, props: unknown): Promise<T> { | ||
| return this.fetch(this.buildUrl(path), { | ||
| method: 'POST', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify(props), | ||
| }); | ||
| async postJson<T>(path: string, props: unknown, opts?: RequestOptions): Promise<T> { | ||
| return this.fetch( | ||
| this.buildUrl(path), | ||
| { | ||
| method: 'POST', | ||
| headers: { 'content-type': 'application/json' }, | ||
| body: JSON.stringify(props), | ||
| }, | ||
| opts, | ||
| ); | ||
| } | ||
|
|
||
| protected buildUrl(path: string, query: Record<string, unknown> = {}): URL { | ||
|
|
@@ -94,21 +99,42 @@ export abstract class BaseApiClient { | |
| } catch { | ||
| /* empty */ | ||
| } | ||
| return new TonClientError(`HTTP ${response.status}: ${message}`, code, detail); | ||
| return new ApiClientHttpError(`HTTP ${response.status}: ${message}`, code, detail); | ||
| } | ||
|
|
||
| private async doRequest(url: URL, init: globalThis.RequestInit = {}): Promise<globalThis.Response> { | ||
| private async doRequest( | ||
| url: URL, | ||
| init: globalThis.RequestInit = {}, | ||
| externalSignal?: AbortSignal, | ||
| ): Promise<globalThis.Response> { | ||
| const fetchFn = this.fetchApi; | ||
|
|
||
| if (!this.timeout || this.timeout <= 0) { | ||
| return fetchFn(url, init); | ||
| // Bail out before touching the network if the caller already aborted. | ||
| externalSignal?.throwIfAborted(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The AbortSignal.throwIfAborted method is supported in the following environments: Browser Support: - Safari: Supported since version 15.4 [1]. - iOS WebView (WKWebView): Supported since version 15.4 [1]. - Android WebView: Supported since version 149 [1]. Node.js Support: - Node.js: Supported since v16.17.0 and v17.3.0 [2][3][4][5]. Summary Table: Environment | Minimum Version --- | --- Safari | 15.4 [1] iOS WebView | 15.4 [1] Android WebView | 149 [1] Node.js | 16.17.0 / 17.3.0 [2][3][4][5] Note: While AbortController and the basic AbortSignal interface have broader support in older versions of these platforms (e.g., Android WebView 66+ for AbortController) [6][7], the specific throwIfAborted convenience method was introduced later [1][8]. Citations:
🏁 Script executed: #!/bin/bash
# Check for runtime configuration in project files
for file in "tsconfig.json" "package.json" ".browserslistrc" "webpack.config.js" "babel.config.js"; do
if [ -f "$file" ]; then
echo "=== $file ==="
cat "$file" | head -50
fi
doneRepository: ton-org/kit Length of output: 1944 🏁 Script executed: #!/bin/bash
# Examine the BaseApiClient to understand signal handling context
wc -l packages/walletkit/src/clients/BaseApiClient.ts
cat -n packages/walletkit/src/clients/BaseApiClient.ts | head -150Repository: ton-org/kit Length of output: 1944 🌐 Web query:
💡 Result: The AbortSignal.aborted property is widely supported across modern web browsers and server-side environments [1][2]. It has been a stable, established feature since early 2018 [1]. Runtime Compatibility: - Safari and iOS Safari: Supported since version 11.1 [3][2]. - Android WebView: Supported since version 66 [2]. - Chrome and Chrome for Android: Supported since version 66 [3][2]. - Node.js: Supported since version 15.0.0 and 14.17.0 [4][5]. The property is a read-only boolean that returns true if the signal has been aborted and false otherwise [1][6]. It is commonly used alongside AbortController to manage and cancel asynchronous operations [1][4]. Because it is a well-established standard, it is widely available in all modern browsers and Node.js environments [1][4][2]. Citations:
Use Line 113 uses 🐛 Proposed fix- externalSignal?.throwIfAborted();
+ if (externalSignal?.aborted) {
+ throw externalSignal.reason ?? new Error('Aborted');
+ }🤖 Prompt for AI Agents |
||
|
|
||
| const hasTimeout = this.timeout > 0; | ||
| if (!hasTimeout) { | ||
| return fetchFn(url, externalSignal ? { ...init, signal: externalSignal } : init); | ||
| } | ||
|
|
||
| // Fresh controller per attempt — a timeout aborts it, so it cannot be | ||
| // reused once spent. A flag (not the abort reason) distinguishes a | ||
| // timeout from a caller abort, so the underlying AbortError surfaces as | ||
| // a typed ApiClientTimeoutError while the caller's abort propagates as-is. | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), this.timeout); | ||
| let timedOut = false; | ||
| const timeoutId = setTimeout(() => { | ||
| timedOut = true; | ||
| controller.abort(); | ||
| }, this.timeout); | ||
|
|
||
| try { | ||
| return await fetchFn(url, { ...init, signal: controller.signal }); | ||
| return await fetchFn(url, { ...init, signal: combineSignals(externalSignal, controller.signal) }); | ||
| } catch (error) { | ||
| if (timedOut) { | ||
| throw new ApiClientTimeoutError(`Request timed out after ${this.timeout}ms`, this.timeout); | ||
| } | ||
| throw error; | ||
| } finally { | ||
| clearTimeout(timeoutId); | ||
| } | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| /** | ||
| * Copyright (c) TonTech. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| */ | ||
|
|
||
| /** | ||
| * Combines a caller-provided abort signal with the client's internal timeout | ||
| * signal into one: the result aborts when *either* fires. Uses native | ||
| * `AbortSignal.any` where available, falling back to manual listener wiring on | ||
| * older runtimes (some mobile WebViews) so a caller's abort is still honored. | ||
| */ | ||
| export function combineSignals(external: AbortSignal | undefined, internal: AbortSignal): AbortSignal { | ||
| if (!external) { | ||
| return internal; | ||
| } | ||
| if (typeof AbortSignal !== 'undefined' && typeof AbortSignal.any === 'function') { | ||
| return AbortSignal.any([external, internal]); | ||
| } | ||
| const controller = new AbortController(); | ||
| const abortWith = (signal: AbortSignal) => () => { | ||
| if (!controller.signal.aborted) { | ||
| controller.abort(signal.reason); | ||
| } | ||
| }; | ||
| if (external.aborted) { | ||
| controller.abort(external.reason); | ||
| } else if (internal.aborted) { | ||
| controller.abort(internal.reason); | ||
| } else { | ||
| external.addEventListener('abort', abortWith(external), { once: true }); | ||
| internal.addEventListener('abort', abortWith(internal), { once: true }); | ||
| } | ||
| return controller.signal; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /** | ||
| * Copyright (c) TonTech. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| */ | ||
|
|
||
| /** | ||
| * Base class for every error surfaced by the API clients. Catch this to handle | ||
| * any client-originated failure uniformly; narrow to a subclass for specifics. | ||
| */ | ||
| export class ApiClientError extends Error { | ||
| constructor(message: string, options?: ErrorOptions) { | ||
| super(message, options); | ||
| this.name = 'ApiClientError'; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The server responded, but with a non-2xx status (or an unexpected body). | ||
| * Carries the HTTP `status` and any parsed error `details`. | ||
| */ | ||
| export class ApiClientHttpError extends ApiClientError { | ||
| public readonly status: number; | ||
| public readonly details?: unknown; | ||
|
|
||
| constructor(message: string, status: number, details?: unknown) { | ||
| super(message); | ||
| this.name = 'ApiClientHttpError'; | ||
| this.status = status; | ||
| this.details = details; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The request did not complete within the configured `timeout`. Distinct from a | ||
| * caller-initiated abort: a timeout is retryable (when `retryOnTimeout` is set), | ||
| * an abort never is. | ||
| */ | ||
| export class ApiClientTimeoutError extends ApiClientError { | ||
| public readonly timeoutMs?: number; | ||
|
|
||
| constructor(message = 'Request timed out', timeoutMs?: number) { | ||
| super(message); | ||
| this.name = 'ApiClientTimeoutError'; | ||
| this.timeoutMs = timeoutMs; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The request never reached a response — a transport-level failure (DNS, refused | ||
| * connection, offline, TLS). There is no HTTP status. The original `fetch` | ||
| * `TypeError` is preserved as `cause`. | ||
| */ | ||
| export class ApiClientNetworkError extends ApiClientError { | ||
| constructor(message: string, options?: ErrorOptions) { | ||
| super(message, options); | ||
| this.name = 'ApiClientNetworkError'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align
getPendingTransactionssignature with concrete TON API client.ApiClientnow declaresgetPendingTransactions(request, opts?), butApiClientTonApistill exposes a single-argument signature. Consumers typed asApiClientTonApican’t passRequestOptionson this method, which breaks the new per-request contract consistency.💡 Suggested fix (in
packages/walletkit/src/clients/tonapi/ApiClientTonApi.ts)🤖 Prompt for AI Agents