Draft
Conversation
📝 WalkthroughWalkthroughThis pull request adds comprehensive abort signal support across the TypeScript SDK, enabling request cancellation for both gRPC and REST API calls. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as gRPC/REST API
participant Retry as Retry Handler
participant HTTP as HTTP Client
participant Server
Client->>API: fetchData(params, {signal: abortController.signal})
API->>Retry: execute with signal
alt Signal Already Aborted
Retry->>Retry: Check signal.aborted
Retry-->>API: Throw AbortError
API-->>Client: Rejected Promise
else Signal Active
Retry->>HTTP: Call request with signal
HTTP->>Server: Send request
alt Request Completes
Server-->>HTTP: Response
HTTP-->>Retry: Return data
Retry-->>API: Return result
API-->>Client: Resolved Promise
else Signal Aborted During Request
Client->>Client: Abort triggered
Client->>HTTP: Cancel via signal
HTTP->>Server: Cancel request
HTTP-->>Retry: Throw AbortError
Retry-->>API: Throw AbortError
API-->>Client: Rejected Promise
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk-ts/src/client/chain/rest/ChainRestWasmApi.ts (1)
40-44:⚠️ Potential issue | 🟡 MinorIncorrect
contextModulevalue.The error context uses
ChainModule.Bankbut this is the Wasm API. This will cause misleading error context when exceptions are thrown. Should useChainModule.Wasmto match the pattern used in other APIs (e.g.,ChainGrpcWasmApiusesChainModule.Wasm).🐛 Proposed fix
throw new HttpRequestException(new Error(e as any), { code: UnspecifiedErrorCode, context: `${this.endpoint}/${endpoint}`, - contextModule: ChainModule.Bank, + contextModule: ChainModule.Wasm, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-ts/src/client/chain/rest/ChainRestWasmApi.ts` around lines 40 - 44, The thrown HttpRequestException in ChainRestWasmApi is using the wrong module context (ChainModule.Bank) which mislabels Wasm errors; update the exception construction in ChainRestWasmApi so the contextModule is ChainModule.Wasm (keep the rest: HttpRequestException(new Error(e as any), { code: UnspecifiedErrorCode, context: `${this.endpoint}/${endpoint}`, contextModule: ChainModule.Wasm })) to match ChainGrpcWasmApi and other Wasm handlers.
🧹 Nitpick comments (4)
packages/sdk-ts/src/core/tx/api/TxRestApi.ts (2)
184-227: Consider documenting the dual options parameters.The
broadcastmethod accepts bothoptions?: TxClientBroadcastOptions(for timeout/txTimeout) andcallOptions?: CallOptions(for abort signal). While this maintains backward compatibility, a brief JSDoc comment would help clarify the distinction between these two parameter groups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-ts/src/core/tx/api/TxRestApi.ts` around lines 184 - 227, Add a concise JSDoc to the broadcast method clarifying the two separate option objects: explain that options?: TxClientBroadcastOptions controls timeouts (timeout and txTimeout) and is used to compute the polling timeout, while callOptions?: CallOptions carries request-level settings such as an AbortSignal for cancellation passed to broadcastTx and fetchTxPoll; annotate parameters and mention default behavior (DEFAULT_BLOCK_TIMEOUT_HEIGHT/DEFAULT_BLOCK_TIME_IN_SECONDS) and which internal calls use each (broadcast -> broadcastTx uses callOptions.signal, fetchTxPoll uses computed timeout). Ensure the JSDoc appears immediately above the public async broadcast(...) signature and references the TxClientBroadcastOptions and CallOptions types by name.
106-147: Consider adding abort listener during polling delay.The abort check at Line 114 only triggers at the start of each loop iteration. If the signal is aborted during the
setTimeoutdelay (Line 135), the loop will complete the current sleep before checking the signal again.For more responsive cancellation, consider using an abort-aware delay:
♻️ Optional: Abort-aware delay for faster cancellation
- await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL)) + await new Promise((resolve, reject) => { + const timeout = setTimeout(resolve, POLL_INTERVAL) + options?.signal?.addEventListener('abort', () => { + clearTimeout(timeout) + reject( + options.signal!.reason ?? + new DOMException('The operation was aborted.', 'AbortError'), + ) + }, { once: true }) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-ts/src/core/tx/api/TxRestApi.ts` around lines 106 - 147, The polling loop in fetchTxPoll currently checks options?.signal?.aborted only at the top of each iteration, so the await new Promise(resolve => setTimeout(resolve, POLL_INTERVAL)) call can delay abort handling; update the sleep to be abort-aware by replacing that setTimeout Promise with one that listens to options.signal (or uses Promise.race with an abort Promise) and rejects or resolves immediately when the signal is aborted, referencing fetchTxPoll, POLL_INTERVAL and options.signal to locate where to change the delay so cancellation is handled promptly during the polling wait.packages/sdk-ts/src/client/indexer/rest/IndexerRestExplorerApi.ts (1)
54-64: Inconsistent use ofendpointvariable.The
endpointvariable is defined on line 54 but thethis.get()call on line 60 duplicates the template literal instead of reusing the variable. Other methods in this file (e.g.,fetchBlocks,fetchBlocksWithTx) use the endpoint variable consistently.♻️ Suggested fix
- () => this.get(`blocks/${blockHashHeight}`, {}, options?.signal), + () => this.get(endpoint, {}, options?.signal),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-ts/src/client/indexer/rest/IndexerRestExplorerApi.ts` around lines 54 - 64, The code defines endpoint = `blocks/${blockHashHeight}` but then calls this.get with an inlined template literal; update the retry call in IndexerRestExplorerApi (the method that fetches a single block) to reuse the endpoint variable instead of duplicating the template literal — i.e., replace the inlined string in the this.get(...) invocation inside the retry(...) call with the endpoint identifier so it matches other methods like fetchBlocks/fetchBlocksWithTx.packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts (1)
286-290: Consider object-based parameters for better ergonomics (future improvement).Methods with multiple optional positional parameters before
options(likefetchDerivativeMarkets,fetchSpotMarkets,fetchFullSpotMarkets) require callers to passundefinedfor unused parameters when only specifying the abort signal:api.fetchDerivativeMarkets(undefined, undefined, { signal: controller.signal })For a future refactor, consider consolidating into an options object pattern. This is not blocking for this PR since it maintains consistency with the existing API structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts` around lines 286 - 290, Change the multi-optional positional parameters to a single options object to improve ergonomics: replace the signature of fetchDerivativeMarkets(status?: string, marketIds?: string[], options?: GrpcCallOptions) with a single parameter like fetchDerivativeMarkets(opts?: { status?: string; marketIds?: string[] } & GrpcCallOptions) and update internal references to read opts.status, opts.marketIds, and opts.signal; apply the same pattern to fetchSpotMarkets and fetchFullSpotMarkets so callers can call api.fetchDerivativeMarkets({ marketIds: [...], signal: controller.signal }) without passing undefined for unused args.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts`:
- Around line 129-151: The abort listener added to the AbortSignal in
BaseGrpcConsumer.retryGrpcCall is not removed when the timeout fires or when the
retry resolves, which can leak listeners if the same AbortController is reused;
fix by using a named handler (e.g., const onAbort = () => { ... }) passed to
signal.addEventListener and call signal.removeEventListener(onAbort) before
resolving the Promise in the timeout callback (clearTimeout already exists) and
also remove the listener whenever you resolve or reject the Promise from other
code paths so the onAbort handler is always detached.
In `@packages/utils/src/classes/HttpClient.ts`:
- Line 37: The HTTP methods currently spread this.config after method-level
options so config can override per-request cancellation; update each HTTP method
(the calls to this.client.get, this.client.post, this.client.put,
this.client.delete) to spread ...this.config first and then pass method-specific
options (params, signal, body, etc.) so the method-level signal and other
options always take precedence over the instance config.
In `@packages/utils/src/classes/HttpRestClient.ts`:
- Around line 122-144: The retry promise currently adds an 'abort' listener to
signal but never removes it when the timeout fires, causing orphaned listeners;
update retryHttpCall's Promise so the abort listener is a named function (e.g.,
onAbort) and call signal.removeEventListener('abort', onAbort) before resolving
in the timeout callback (and also after rejecting if you keep the existing
inline reject path) and clear the timeout via timeoutId as you already do;
ensure the listener is registered without relying solely on { once: true } so it
can be explicitly removed on successful retry to prevent memory leaks.
---
Outside diff comments:
In `@packages/sdk-ts/src/client/chain/rest/ChainRestWasmApi.ts`:
- Around line 40-44: The thrown HttpRequestException in ChainRestWasmApi is
using the wrong module context (ChainModule.Bank) which mislabels Wasm errors;
update the exception construction in ChainRestWasmApi so the contextModule is
ChainModule.Wasm (keep the rest: HttpRequestException(new Error(e as any), {
code: UnspecifiedErrorCode, context: `${this.endpoint}/${endpoint}`,
contextModule: ChainModule.Wasm })) to match ChainGrpcWasmApi and other Wasm
handlers.
---
Nitpick comments:
In `@packages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.ts`:
- Around line 286-290: Change the multi-optional positional parameters to a
single options object to improve ergonomics: replace the signature of
fetchDerivativeMarkets(status?: string, marketIds?: string[], options?:
GrpcCallOptions) with a single parameter like fetchDerivativeMarkets(opts?: {
status?: string; marketIds?: string[] } & GrpcCallOptions) and update internal
references to read opts.status, opts.marketIds, and opts.signal; apply the same
pattern to fetchSpotMarkets and fetchFullSpotMarkets so callers can call
api.fetchDerivativeMarkets({ marketIds: [...], signal: controller.signal })
without passing undefined for unused args.
In `@packages/sdk-ts/src/client/indexer/rest/IndexerRestExplorerApi.ts`:
- Around line 54-64: The code defines endpoint = `blocks/${blockHashHeight}` but
then calls this.get with an inlined template literal; update the retry call in
IndexerRestExplorerApi (the method that fetches a single block) to reuse the
endpoint variable instead of duplicating the template literal — i.e., replace
the inlined string in the this.get(...) invocation inside the retry(...) call
with the endpoint identifier so it matches other methods like
fetchBlocks/fetchBlocksWithTx.
In `@packages/sdk-ts/src/core/tx/api/TxRestApi.ts`:
- Around line 184-227: Add a concise JSDoc to the broadcast method clarifying
the two separate option objects: explain that options?: TxClientBroadcastOptions
controls timeouts (timeout and txTimeout) and is used to compute the polling
timeout, while callOptions?: CallOptions carries request-level settings such as
an AbortSignal for cancellation passed to broadcastTx and fetchTxPoll; annotate
parameters and mention default behavior
(DEFAULT_BLOCK_TIMEOUT_HEIGHT/DEFAULT_BLOCK_TIME_IN_SECONDS) and which internal
calls use each (broadcast -> broadcastTx uses callOptions.signal, fetchTxPoll
uses computed timeout). Ensure the JSDoc appears immediately above the public
async broadcast(...) signature and references the TxClientBroadcastOptions and
CallOptions types by name.
- Around line 106-147: The polling loop in fetchTxPoll currently checks
options?.signal?.aborted only at the top of each iteration, so the await new
Promise(resolve => setTimeout(resolve, POLL_INTERVAL)) call can delay abort
handling; update the sleep to be abort-aware by replacing that setTimeout
Promise with one that listens to options.signal (or uses Promise.race with an
abort Promise) and rejects or resolves immediately when the signal is aborted,
referencing fetchTxPoll, POLL_INTERVAL and options.signal to locate where to
change the delay so cancellation is handled promptly during the polling wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21f86ede-ba4e-4d57-8aef-acb47801fb17
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
packages/sdk-ts/src/client/abacus/grpc/AbacusGrpcApi.tspackages/sdk-ts/src/client/base/BaseGrpcConsumer.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuctionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcAuthZApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcBankApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcDistributionApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcErc20Api.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcEvmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcExchangeApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcGovApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcIbcApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcMintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcOracleApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPeggyApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcPermissionsApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcStakingApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTendermintApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTokenFactoryApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcTxFeesApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmApi.tspackages/sdk-ts/src/client/chain/grpc/ChainGrpcWasmXApi.tspackages/sdk-ts/src/client/chain/rest/ChainRestAuthApi.tspackages/sdk-ts/src/client/chain/rest/ChainRestBankApi.tspackages/sdk-ts/src/client/chain/rest/ChainRestTendermintApi.tspackages/sdk-ts/src/client/chain/rest/ChainRestWasmApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAccountApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcArchiverApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcAuctionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcCampaignApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcDerivativesApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcExplorerApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcInsuranceFundApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMegaVaultApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMetaApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcMitoApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcOracleApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcPortfolioApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcReferralApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcRfqApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcSpotApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTcDerivativesApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTradingApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcTransactionApi.tspackages/sdk-ts/src/client/indexer/grpc/IndexerGrpcWeb3GwApi.tspackages/sdk-ts/src/client/indexer/rest/IndexerRestDerivativesChronosApi.tspackages/sdk-ts/src/client/indexer/rest/IndexerRestExplorerApi.tspackages/sdk-ts/src/client/indexer/rest/IndexerRestLeaderboardChronosApi.tspackages/sdk-ts/src/client/indexer/rest/IndexerRestMarketChronosApi.tspackages/sdk-ts/src/client/indexer/rest/IndexerRestSpotChronosApi.tspackages/sdk-ts/src/client/olp/grpc/OLPGrpcApi.tspackages/sdk-ts/src/client/tcAbacus/grpc/TcAbacusGrpcApi.tspackages/sdk-ts/src/core/tx/api/TxRestApi.tspackages/sdk-ts/src/types/grpc.tspackages/sdk-ts/src/utils/pagination.tspackages/utils/src/classes/HttpClient.tspackages/utils/src/classes/HttpRestClient.ts
Comment on lines
+129
to
+151
| return new Promise<TResponse>((resolve, reject) => { | ||
| const timeoutId = setTimeout( | ||
| () => resolve(retryGrpcCall(attempt + 1)), | ||
| delay * attempt, | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| if (signal) { | ||
| signal.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| clearTimeout(timeoutId) | ||
| reject( | ||
| signal.reason ?? | ||
| new DOMException( | ||
| 'The operation was aborted.', | ||
| 'AbortError', | ||
| ), | ||
| ) | ||
| }, | ||
| { once: true }, | ||
| ) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Potential memory leak: abort listener is not removed on successful retry or timeout.
When the retry succeeds or the timeout fires naturally, the abort listener registered on the signal is not cleaned up. If the same AbortController is reused across multiple calls, listeners accumulate.
🛡️ Proposed fix to clean up the abort listener
return new Promise<TResponse>((resolve, reject) => {
+ const onAbort = () => {
+ clearTimeout(timeoutId)
+ reject(
+ signal.reason ??
+ new DOMException(
+ 'The operation was aborted.',
+ 'AbortError',
+ ),
+ )
+ }
+
const timeoutId = setTimeout(
- () => resolve(retryGrpcCall(attempt + 1)),
+ () => {
+ signal?.removeEventListener('abort', onAbort)
+ resolve(retryGrpcCall(attempt + 1))
+ },
delay * attempt,
)
if (signal) {
- signal.addEventListener(
- 'abort',
- () => {
- clearTimeout(timeoutId)
- reject(
- signal.reason ??
- new DOMException(
- 'The operation was aborted.',
- 'AbortError',
- ),
- )
- },
- { once: true },
- )
+ signal.addEventListener('abort', onAbort, { once: true })
}
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/sdk-ts/src/client/base/BaseGrpcConsumer.ts` around lines 129 - 151,
The abort listener added to the AbortSignal in BaseGrpcConsumer.retryGrpcCall is
not removed when the timeout fires or when the retry resolves, which can leak
listeners if the same AbortController is reused; fix by using a named handler
(e.g., const onAbort = () => { ... }) passed to signal.addEventListener and call
signal.removeEventListener(onAbort) before resolving the Promise in the timeout
callback (clearTimeout already exists) and also remove the listener whenever you
resolve or reject the Promise from other code paths so the onAbort handler is
always detached.
| params: T = {} as T, | ||
| signal?: AbortSignal, | ||
| ): Promise<P> { | ||
| return this.client.get(endpoint, { params, signal, ...this.config }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for setConfig calls that may pass signal/params..."
rg -nU --type=ts '\.setConfig\(\s*\{[\s\S]{0,250}?(signal|params)\s*:' packages
echo
echo "Current HttpClient call config ordering:"
sed -n '32,62p' packages/utils/src/classes/HttpClient.tsRepository: InjectiveLabs/injective-ts
Length of output: 130
🏁 Script executed:
cat -n packages/utils/src/classes/HttpClient.ts | head -70Repository: InjectiveLabs/injective-ts
Length of output: 1838
🏁 Script executed:
rg -nU --type=ts '\.setConfig\(' packages --max-count=20Repository: InjectiveLabs/injective-ts
Length of output: 151
🏁 Script executed:
fd -e ts -e js packages | xargs rg 'new HttpClient' | head -20Repository: InjectiveLabs/injective-ts
Length of output: 333
🏁 Script executed:
cat -n packages/utils/src/classes/HttpRestClient.ts | sed -n '30,45p'Repository: InjectiveLabs/injective-ts
Length of output: 560
🏁 Script executed:
rg -nU --type=ts 'setConfig' packages -B 3 -A 3Repository: InjectiveLabs/injective-ts
Length of output: 1033
🏁 Script executed:
rg -nU --type=ts 'setConfig\(' packages -B 2 -A 5 | head -50Repository: InjectiveLabs/injective-ts
Length of output: 1104
🏁 Script executed:
rg -nU --type=ts 'signal.*:' packages -B 3 -A 1 | grep -A 4 -B 4 'setConfig'Repository: InjectiveLabs/injective-ts
Length of output: 52
🏁 Script executed:
fd -e ts packages | xargs grep -l 'setConfig' | head -10Repository: InjectiveLabs/injective-ts
Length of output: 52
🏁 Script executed:
rg -nU --type=ts 'setConfig\(' --context=8 packagesRepository: InjectiveLabs/injective-ts
Length of output: 2353
🏁 Script executed:
rg -nU --type=ts 'setConfig\s*\(' --context=12 packages | grep -v '^--$' | head -80Repository: InjectiveLabs/injective-ts
Length of output: 3462
🏁 Script executed:
git log --oneline --all -S 'setConfig' -- packages | head -20Repository: InjectiveLabs/injective-ts
Length of output: 156
🏁 Script executed:
cat -n packages/utils/src/classes/HttpRestClient.ts | sed -n '42,80p'Repository: InjectiveLabs/injective-ts
Length of output: 1497
🏁 Script executed:
rg -nU --type=ts 'setConfig.*\{' packages -A 3 | head -40Repository: InjectiveLabs/injective-ts
Length of output: 645
🏁 Script executed:
rg -nU --type=ts 'signal.*AbortSignal' packages | head -20Repository: InjectiveLabs/injective-ts
Length of output: 1206
🏁 Script executed:
rg -nU --type=ts 'setConfig\(' packages --context=6 | grep -A 10 'setConfig' | head -50Repository: InjectiveLabs/injective-ts
Length of output: 1198
🏁 Script executed:
python3 << 'EOF'
# Check JavaScript object spread precedence behavior
# Later properties override earlier ones with same key
test_spread = {
"params": "from_config",
"signal": "from_config",
"timeout": 5000
}
# Order 1: { params, signal, ...this.config }
# This means: params (undefined), signal (method param), then config spreads in
# If config has signal, it wins
# Order 2: { ...this.config, params, signal }
# This means: config spreads first, then params and signal from method override
print("Current order problem:")
print(" { params, signal, ...this.config }")
print(" If this.config has 'signal', it overrides the method parameter")
print()
print("Proposed order:")
print(" { ...this.config, params, signal }")
print(" Method parameters always override config properties")
print()
print("This is a valid concern for defensive programming.")
EOFRepository: InjectiveLabs/injective-ts
Length of output: 354
🏁 Script executed:
cat -n packages/utils/src/classes/HttpClient.ts | sed -n '26,62p'Repository: InjectiveLabs/injective-ts
Length of output: 1156
Reorder spread to ensure method-level signal takes precedence over config.
In all HTTP methods, ...this.config spreads after signal, allowing setConfig() to override per-request cancellation. This is unsafe for request cancellation which is timing-critical. Move ...this.config first so method parameters always win.
Apply to all four methods
- return this.client.get(endpoint, { params, signal, ...this.config })
+ return this.client.get(endpoint, { ...this.config, params, signal })- return this.client.post(endpoint, data, { signal, ...this.config })
+ return this.client.post(endpoint, data, { ...this.config, signal })- return this.client.put(endpoint, data, { signal, ...this.config })
+ return this.client.put(endpoint, data, { ...this.config, signal })- return this.client.delete(endpoint, { params, signal, ...this.config })
+ return this.client.delete(endpoint, { ...this.config, params, signal })Applies to lines 37, 45, 53, 61 in packages/utils/src/classes/HttpClient.ts.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return this.client.get(endpoint, { params, signal, ...this.config }) | |
| return this.client.get(endpoint, { ...this.config, params, signal }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utils/src/classes/HttpClient.ts` at line 37, The HTTP methods
currently spread this.config after method-level options so config can override
per-request cancellation; update each HTTP method (the calls to this.client.get,
this.client.post, this.client.put, this.client.delete) to spread ...this.config
first and then pass method-specific options (params, signal, body, etc.) so the
method-level signal and other options always take precedence over the instance
config.
Comment on lines
+122
to
+144
| return new Promise((resolve, reject) => { | ||
| const timeoutId = setTimeout( | ||
| () => resolve(retryHttpCall(attempt + 1)), | ||
| delay * attempt, | ||
| ), | ||
| ) | ||
| ) | ||
|
|
||
| if (signal) { | ||
| signal.addEventListener( | ||
| 'abort', | ||
| () => { | ||
| clearTimeout(timeoutId) | ||
| reject( | ||
| signal.reason ?? | ||
| new DOMException( | ||
| 'The operation was aborted.', | ||
| 'AbortError', | ||
| ), | ||
| ) | ||
| }, | ||
| { once: true }, | ||
| ) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Potential memory leak: abort listener not removed on successful retry.
When the timeout fires and resolves the promise, the abort event listener remains attached to the signal. If the signal is long-lived and many retries occur, these orphaned listeners accumulate.
🛡️ Proposed fix to clean up the listener
return new Promise((resolve, reject) => {
+ const onAbort = () => {
+ clearTimeout(timeoutId)
+ reject(
+ signal.reason ??
+ new DOMException(
+ 'The operation was aborted.',
+ 'AbortError',
+ ),
+ )
+ }
+
const timeoutId = setTimeout(
- () => resolve(retryHttpCall(attempt + 1)),
+ () => {
+ if (signal) {
+ signal.removeEventListener('abort', onAbort)
+ }
+ resolve(retryHttpCall(attempt + 1))
+ },
delay * attempt,
)
if (signal) {
- signal.addEventListener(
- 'abort',
- () => {
- clearTimeout(timeoutId)
- reject(
- signal.reason ??
- new DOMException(
- 'The operation was aborted.',
- 'AbortError',
- ),
- )
- },
- { once: true },
- )
+ signal.addEventListener('abort', onAbort, { once: true })
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new Promise((resolve, reject) => { | |
| const timeoutId = setTimeout( | |
| () => resolve(retryHttpCall(attempt + 1)), | |
| delay * attempt, | |
| ), | |
| ) | |
| ) | |
| if (signal) { | |
| signal.addEventListener( | |
| 'abort', | |
| () => { | |
| clearTimeout(timeoutId) | |
| reject( | |
| signal.reason ?? | |
| new DOMException( | |
| 'The operation was aborted.', | |
| 'AbortError', | |
| ), | |
| ) | |
| }, | |
| { once: true }, | |
| ) | |
| } | |
| }) | |
| return new Promise((resolve, reject) => { | |
| const onAbort = () => { | |
| clearTimeout(timeoutId) | |
| reject( | |
| signal.reason ?? | |
| new DOMException( | |
| 'The operation was aborted.', | |
| 'AbortError', | |
| ), | |
| ) | |
| } | |
| const timeoutId = setTimeout( | |
| () => { | |
| if (signal) { | |
| signal.removeEventListener('abort', onAbort) | |
| } | |
| resolve(retryHttpCall(attempt + 1)) | |
| }, | |
| delay * attempt, | |
| ) | |
| if (signal) { | |
| signal.addEventListener('abort', onAbort, { once: true }) | |
| } | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utils/src/classes/HttpRestClient.ts` around lines 122 - 144, The
retry promise currently adds an 'abort' listener to signal but never removes it
when the timeout fires, causing orphaned listeners; update retryHttpCall's
Promise so the abort listener is a named function (e.g., onAbort) and call
signal.removeEventListener('abort', onAbort) before resolving in the timeout
callback (and also after rejecting if you keep the existing inline reject path)
and clear the timeout via timeoutId as you already do; ensure the listener is
registered without relying solely on { once: true } so it can be explicitly
removed on successful retry to prevent memory leaks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request adds support for passing an abort signal to gRPC API calls across several modules, enabling consumers to cancel in-flight requests more easily. It introduces an optional
GrpcCallOptionsparameter (with anAbortSignal) to relevant API methods and propagates this support through the base gRPC consumer logic, including proper handling of abort events and error propagation.Key changes include:
API Method Enhancements:
GrpcCallOptionsparameter (withsignal) to most public gRPC API methods inAbacusGrpcApi,ChainGrpcAuctionApi,ChainGrpcAuthApi,ChainGrpcAuthZApi, andChainGrpcBankApi, allowing consumers to cancel requests. [1] [2] [3] [4] [5]Base gRPC Consumer Improvements:
BaseGrpcConsumerto accept an optionalAbortSignalin itsgetRpcOptions,retry, andexecuteGrpcCallmethods. Now, if a signal is aborted, the call is cancelled and an appropriate error is thrown. [1] [2] [3]handleGrpcErrorto treat abort errors as gRPC cancellations, providing a consistent exception for consumers.Method Signatures and Internal Usage:
options?.signalparameter down to the base consumer logic. [1] [2] [3] [4] [5] [6] [7] [8] [9]These changes make the SDK more robust and responsive, especially in environments where request cancellation is important (such as UI applications or serverless functions).
References:
[1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14]
Summary by CodeRabbit