Adds abort signal to requests #368

francislavoie · 2025-12-09T06:31:44Z

Fixes #367

Enables request abortion using AbortSignal in all transports. Ensures requests can be cancelled, prevents memory leaks and race conditions, by rejecting requests that are no longer relevant.
Uses an AbortError to differentiate RPC errors from aborts.
Ran npm i baseline-browser-mapping@latest -D to update browser mappings (fixes warnings when running tests)
Ran npm audit fix to fix 3 vulnerabilities (1 low, 1 moderate, 1 high).
Improved test coverage to 100%; in addition to abort tests, also adds a test for when fetcher is missing, and a test to cover index.ts exports.

codecov · 2025-12-09T06:32:50Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4312a28) to head (c7b2c2f).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master      #368      +/-   ##
===========================================
+ Coverage   99.75%   100.00%   +0.24%     
===========================================
  Files          11        11              
  Lines         404       434      +30     
  Branches       56        61       +5     
===========================================
+ Hits          403       434      +31     
+ Misses          1         0       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Enables request abortion using AbortSignal in all transports. The change ensures requests can be cancelled, prevents memory leaks and improves user experience by rejecting requests that are no longer relevant. Implements AbortError class to differentiate RPC errors from aborts. Ran `npm i baseline-browser-mapping@latest -D` to update browser mappings, and `npm audit fix` to fix 3 vulnerabilities (1 low, 1 moderate, 1 high).

zcstarr · 2025-12-09T17:01:15Z

Thanks for the PR !!

Going to leave this here, early haven't fully reviewed the PR, but I'd say a few off the top of my head comments.

If we can minimize the amount of changes to just be the Error signal so it's easier to merge in and account for.
We migrated to bun recently, and so packages should be installed with bun ( I think there's a ci issue that keeps adding back a package-lock.json
Is browser based mapping necessary, I think try bun test , if still warning we can always put up a separate PR to fix it ?
I think maybe the fetcher part is bun/build related, as bun bundles as well and that might be causing some issues.

I'll comment further when I get a chance to go over it more in depth hopefully sometime this or early next week

francislavoie · 2025-12-09T17:25:38Z

bun test doesn't pass, fails on all WS and iframe tests

12 tests failed:
✗ WebSocketTransport > can connect [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive data [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive data against potential timeout [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can send and receive errors [5000.03ms]
  ^ this test timed out after 5000ms.
✗ WebSocketTransport > can handle underlying transport crash [5000.03ms]
  ^ this test timed out after 5000ms.
✗ PostMessageIframeTransport > iframe > can connect [1.00ms]
✗ PostMessageIframeTransport > iframe > can close
✗ PostMessageIframeTransport > iframe > can send and receive data
✗ PostMessageIframeTransport > iframe > can send and receive data against potential timeout
✗ PostMessageIframeTransport > iframe > can send and receive errors
✗ PostMessageIframeTransport > iframe > can handle underlying transport crash

npm run test passes just fine.

$ npm run test                                                                          

> @open-rpc/client-js@0.0.0-development test
> jest --coverage

 PASS  src/transports/EventEmitterTransport.test.ts
 PASS  src/Error.test.ts
 PASS  src/transports/HTTPTransport.test.ts
 PASS  src/transports/WebSocketTransport.test.ts
 PASS  src/transports/PostMessageIframeTransport.test.ts
[baseline-browser-mapping] The data in this module is over two months old.  To ensure accurate Baseline data, please update: `npm i baseline-browser-mapping@latest -D`
 PASS  src/index.test.ts
 PASS  src/transports/TransportRequestManager.test.ts
 PASS  src/RequestManager.test.ts
 PASS  src/transports/PostMessageWindowTransport.test.ts (23.197 s)
--------------------------------|---------|----------|---------|---------|-------------------
File                            | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
--------------------------------|---------|----------|---------|---------|-------------------
All files                       |     100 |      100 |     100 |     100 |                   
 src                            |     100 |      100 |     100 |     100 |                   
  Client.ts                     |     100 |      100 |     100 |     100 |                   
  Request.ts                    |     100 |      100 |     100 |     100 |                   
  RequestManager.ts             |     100 |      100 |     100 |     100 |                   
  index.ts                      |     100 |      100 |     100 |     100 |                   
 src/transports                 |     100 |      100 |     100 |     100 |                   
  EventEmitterTransport.ts      |     100 |      100 |     100 |     100 |                   
  HTTPTransport.ts              |     100 |      100 |     100 |     100 |                   
  PostMessageIframeTransport.ts |     100 |      100 |     100 |     100 |                   
  PostMessageWindowTransport.ts |     100 |      100 |     100 |     100 |                   
  Transport.ts                  |     100 |      100 |     100 |     100 |                   
  TransportRequestManager.ts    |     100 |      100 |     100 |     100 |                   
  WebSocketTransport.ts         |     100 |      100 |     100 |     100 |                   
--------------------------------|---------|----------|---------|---------|-------------------

Test Suites: 9 passed, 9 total
Tests:       87 passed, 87 total
Snapshots:   0 total
Time:        24.34 s
Ran all test suites.

Installing baseline is to get rid of that [baseline-browser-mapping] The data in this module is over two months old. To ensure accurate Baseline data, please update: npm i baseline-browser-mapping@latest -D warning of course

If we can minimize the amount of changes to just be the Error signal so it's easier to merge in and account for.

It's just a couple more tests to complete missing coverage, nothing else.

README.md

src/transports/HTTPTransport.test.ts

src/ClientInterface.ts

zcstarr · 2025-12-16T23:05:56Z

src/transports/Transport.ts

 >;

+export interface TransportOptions {
+  timeout?: number | null;


I think we don't have to move timeout into this object right now. Maybe we shouldn't as we'd introduce a breaking change as you mentioned as well. I'll ponder it for a short spell.

This is also because we might want to store more information in it. I think @shanejonas was at one point thinking about places to store/add more context to the request. I think that's also partially why there's this kind of tension to call it context vs. options. Maybe he can weigh in more on the naming here :D .

Maybe that's misleading in some way?

Is it a breaking change? Does anyone actually provide their own transports?

It's used out in the wild here's an example https://github.com/MetaMask/casper-js-sdk/blob/6628031d6f6fc1279e6748f4722263f724cd2511/src/services/ProviderTransport.ts#L2

I think the solution suggested works well , and we can then streamline it later.

zcstarr · 2026-01-02T19:43:52Z

src/Client.ts

   * myClient.request({method: "foo", params: ["bar"]}).then(() => console.log('foobar'));
   */
-  public async request(requestObject: RequestArguments, timeout?: number) {
+  public async request(requestObject: RequestArguments, options?: Options | number) {


I think this makes sense then we can start to slowly phase out the number as a arg

zcstarr

I think overall it looks good just some adjustments with timing intervals, I think would make testing more reliable with a bigger window for timing jitter.

The only other thing here is a different name maybe than options, my only beef with just options is that it might be too generic, and it'd be nice to scope what can go into options or should go there. I'll give it a little more thought, I'm open to leaving it options want to here if maybe @shanejonas has any alternate ideas

zcstarr · 2026-01-02T19:54:55Z

src/transports/PostMessageIframeTransport.test.ts

      );
      expect(result).toEqual("bar");
-      await new Promise((resolve) => setTimeout(resolve, 100));
+      await new Promise((resolve) => setTimeout(resolve, 10));


I'd keep these at 100 and reset the timeout to be 1000ms . I think this will make sure that slower machines don't catch event loop timing issues.

zcstarr · 2026-01-02T19:55:14Z

src/transports/PostMessageWindowTransport.test.ts

          internalID: 0,
        },
-        10000,
+        { timeout: 100 },


I'd maybe bump this to 1000 there's a significant potential for lag/jitter if I recall about post message window transport at least 1000 seems safer and maybe the timeouts that are set to bump back up to 100.

zcstarr · 2026-01-02T20:02:17Z

src/transports/WebSocketTransport.ts

    // eslint-disable-next-line @typescript-eslint/no-explicit-any
  ): Promise<any> {
-    let prom = this.transportRequestManager.addRequest(data, timeout);
+    const timeout = options?.timeout ?? 5000;


null here should set the timeout to be null, here it seems like we'll always return 5000 , undefined defaults to 5000

Suggested change

const timeout = options?.timeout ?? 5000;

const timeout = options?.timeout === undefined ? 5000 : options.timeout;

zcstarr · 2026-01-02T20:08:10Z

src/transports/TransportRequestManager.ts

      return Promise.resolve();
    }
-    return this.addReq(data.internalID, timeout);
+    return this.addReq(data.internalID, timeout || null, signal);


I'm not sure this makes total sense here, timeout || null , I think it's not necessary as timeout can be number or null and the private method addReq handles the timeout

src/transports/HTTPTransport.test.ts

zcstarr · 2026-01-02T20:30:28Z

src/transports/Transport.ts

 >;

+export interface TransportOptions {
+  timeout?: number | null;


It's used out in the wild here's an example https://github.com/MetaMask/casper-js-sdk/blob/6628031d6f6fc1279e6748f4722263f724cd2511/src/services/ProviderTransport.ts#L2

zcstarr · 2026-01-02T20:31:08Z

src/transports/Transport.ts

 >;

+export interface TransportOptions {
+  timeout?: number | null;


I think the solution suggested works well , and we can then streamline it later.

francislavoie force-pushed the abort branch from 538375c to 39857be Compare December 9, 2025 06:43

francislavoie force-pushed the abort branch from 39857be to 8bb873d Compare December 9, 2025 06:54

use bun install, drop package-lock.json again

0bf521e

shanejonas reviewed Dec 16, 2025

View reviewed changes

README.md Outdated Show resolved Hide resolved

zcstarr reviewed Dec 16, 2025

View reviewed changes

src/transports/HTTPTransport.test.ts Show resolved Hide resolved

Move timeout/signal to Options struct

98d225b

zcstarr reviewed Dec 16, 2025

View reviewed changes

src/ClientInterface.ts Show resolved Hide resolved

francislavoie added 2 commits December 16, 2025 17:31

Re-introduce backwards compat for numeric timeout 2nd arg

2eba1ea

Switch transport to an options struct

c7b2c2f

zcstarr reviewed Dec 16, 2025

View reviewed changes

src/ClientInterface.ts Show resolved Hide resolved

zcstarr reviewed Dec 16, 2025

View reviewed changes

zcstarr reviewed Jan 2, 2026

View reviewed changes

	const timeout = options?.timeout ?? 5000;
	const timeout = options?.timeout === undefined ? 5000 : options.timeout;

Adds abort signal to requests #368

Are you sure you want to change the base?

Adds abort signal to requests #368

Uh oh!

Conversation

francislavoie commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zcstarr commented Dec 9, 2025

Uh oh!

francislavoie commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zcstarr Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zcstarr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

francislavoie commented Dec 9, 2025 •

edited

Loading

codecov bot commented Dec 9, 2025 •

edited

Loading

francislavoie commented Dec 9, 2025 •

edited

Loading

zcstarr Dec 16, 2025 •

edited

Loading