Skip to content

[refactor!]: removed protected abortController + _handleDestory methods… #82

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

Open
js2me wants to merge 1 commit into
base: master
Choose a base branch
from
+70 −26
Open

[refactor!]: removed protected abortController + _handleDestory methods… #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions .changeset/refactor-destroyable-internals.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
"mobx-tanstack-query": major
---

### Breaking changes

- **Removed `protected abortController`** from `Query`, `InfiniteQuery`, and `Mutation`. Use `this._abortSignal` (can be `undefined`) and `this._destroyed` instead.

- **Removed `handleDestroy()`**. Override `destroy()` and call `super.destroy()` at the top:

```ts
// Before
protected handleDestroy() { ... }

// After
destroy() {
super.destroy();
// your cleanup
}
```

- **`Destroyable` no longer calls `makeObservable(this)`**. Call it explicitly in your subclass constructor if needed.
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@
"mobx": "^6.12.4"
},
"dependencies": {
"linked-abort-controller": "^1.1.1",
"yummies": "^7.19.4"
},
"devDependencies": {
"linked-abort-controller": "^1.1.1",
"@biomejs/biome": "^2.4.14",
"@changesets/changelog-github": "^0.6.0",
"@changesets/cli": "^2.31.0",
Expand Down
6 changes: 3 additions & 3 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions src/base-query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ export abstract class BaseQuery<
* [**Documentation**](https://js2me.github.io/mobx-tanstack-query/api/Query.html#update-options)
*/
update(optionsUpdate: TUpdateOptions): void {
if (this.abortController.signal.aborted) {
if (this._destroyed) {
return;
}

Expand Down Expand Up @@ -388,7 +388,7 @@ export abstract class BaseQuery<
}
this.update({} as TUpdateOptions);
}
if (this.abortController.signal.aborted && this._result) {
if (this._destroyed && this._result) {
return this.queryObserver.getOptimisticResult(this.options);
}
return this._result || this.queryObserver.getCurrentResult();
Expand Down Expand Up @@ -587,7 +587,7 @@ export abstract class BaseQuery<

if (this.result.isFetching) {
await when(() => !this.result.isFetching, {
signal: this.abortController.signal,
signal: this._abortSignal,
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot May 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: this._abortSignal can be undefined when no external abort signal is provided to the constructor. This means when() won't be cancelled on destroy(), leaving a dangling observation/promise. The mutation class handles this with this._abortSignal ?? this.recreateTempAc().signal as a fallback.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/base-query.ts, line 590:

<comment>`this._abortSignal` can be `undefined` when no external abort signal is provided to the constructor. This means `when()` won't be cancelled on `destroy()`, leaving a dangling observation/promise. The mutation class handles this with `this._abortSignal ?? this.recreateTempAc().signal` as a fallback.</comment>

<file context>
@@ -587,7 +587,7 @@ export abstract class BaseQuery<
     if (this.result.isFetching) {
       await when(() => !this.result.isFetching, {
-        signal: this.abortController.signal,
+        signal: this._abortSignal,
       });
       const throwableError = this.getCurrentThrowableError();
</file context>
Fix with Cubic

});
const throwableError = this.getCurrentThrowableError();
if (throwableError) {
Expand Down
3 changes: 2 additions & 1 deletion src/inifinite-query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,8 @@ export class InfiniteQuery<
return this._result.fetchPreviousPage(options);
}

protected handleDestroy() {
destroy(): void {
super.destroy();
this.cleanup();
this.hooks?.onInfiniteQueryDestroy?.(this);
}
Expand Down
26 changes: 24 additions & 2 deletions src/mutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ export class Mutation<
isPaused!: boolean;
submittedAt!: number;

/** */
protected tempAc?: AbortController;

constructor(
protected config: MutationConfig<
TData,
Expand Down Expand Up @@ -193,10 +196,14 @@ export class Mutation<
// @ts-expect-error
>(queryClient, {
...this.mutationOptions,
onSettled: (...args) => {
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot May 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: Race condition: when multiple mutations are triggered concurrently (without _abortSignal), the options-level onSettled from an earlier mutation will abort tempAc that belongs to the later in-flight mutation, prematurely signalling abort to it. Consider tracking which tempAc instance the onSettled should abort (e.g., capture a reference at call time) rather than always aborting the current one.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/mutation.ts, line 199:

<comment>Race condition: when multiple mutations are triggered concurrently (without `_abortSignal`), the options-level `onSettled` from an earlier mutation will abort `tempAc` that belongs to the later in-flight mutation, prematurely signalling abort to it. Consider tracking which `tempAc` instance the `onSettled` should abort (e.g., capture a reference at call time) rather than always aborting the current one.</comment>

<file context>
@@ -193,10 +196,14 @@ export class Mutation<
       // @ts-expect-error
     >(queryClient, {
       ...this.mutationOptions,
+      onSettled: (...args) => {
+        this.abortTempAc();
+        return this.mutationOptions.onSettled?.(...args);
</file context>
Fix with Cubic

this.abortTempAc();
return this.mutationOptions.onSettled?.(...args);
},
mutationFn: (variables, context) =>
mutationFn?.(variables, {
...context,
signal: this.abortController.signal,
signal: this._abortSignal ?? this.recreateTempAc().signal,
} satisfies MutationFunctionContext),
});
Comment on lines +199 to 208
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🌐 Web query:

Does @tanstack/query-core MutationObserver 5.90.2 support concurrent in-flight mutations from the same observer, or does mutate() supersede a previous in-flight mutation?

💡 Result:

Yes—MutationObserver (and thus @tanstack/query-core’s mutate/mutateAsync behavior) supports concurrent in-flight mutations from the same MutationObserver; calling mutate does not supersede/cancel a previous in-flight mutation. Evidence: The React TanStack Query docs state that “Per default, all mutations run in parallel - even if you invoke.mutate of the same mutation multiple times,” and that to avoid this you must use a scope.id so they run in serial [1][2]. Additionally, the core MutationObserver implementation shows that each mutate call removes the prior observer link, builds/chooses a mutation from the mutation cache, and then executes it; it doesn’t cancel the previous one, it just updates which mutation the observer is attached to [3]. So if you call observer.mutate(x) then observer.mutate(y) while the first is still pending, both mutation executions can be in-flight concurrently; they are not automatically replaced by the latest call. If you want “only one at a time” semantics, use a scope with an id to queue in serial [1][2], otherwise concurrency is the default.

Citations:

tempAc lifecycle has a cross-mutation race when calls overlap.

@tanstack/query-core's MutationObserver supports concurrent in-flight mutations by default—calling mutate() does not cancel a prior in-flight call. However, this code assumes single-flight semantics and will break under concurrent load:

  1. Overlapping mutations cancel each other's signals. Each mutationFn invocation calls recreateTempAc(), which aborts the previous tempAc. If the same Mutation instance has an in-flight call (A) and mutate() is invoked again (B) before A settles, A's signal is aborted by B's recreateTempAc() even though both are executing concurrently. Then when A eventually settles, the wrapped onSettled calls abortTempAc() and aborts B's signal — corrupting an unrelated in-flight mutation.

  2. Asymmetric signal semantics. When config.abortSignal is provided, all calls share _abortSignal and abortTempAc() becomes a no-op (no per-call cancellation). When it isn't, a fresh tempAc is created per call. The signal a consumer's mutationFn receives therefore behaves differently depending on whether abortSignal was passed in config — this is observable and surprising.

A safer approach is to track the controller per in-flight call (e.g., capture it in a closure inside mutationFn and pass that exact reference to onSettled via a WeakMap keyed on the result/context, or wrap mutationFn itself so each invocation owns its own controller and aborts only that one on settle).

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/mutation.ts` around lines 199 - 208, The current tempAc lifecycle races
because recreateTempAc()/abortTempAc() mutate a single shared tempAc used by all
concurrent mutationFn calls; change to create a per-call AbortController inside
the mutationFn wrapper (capture it in a local variable or store it in a WeakMap
keyed by the mutation context/result) and pass that specific controller.signal
to the underlying mutationFn, then ensure the onSettled wrapper only aborts that
same per-call controller instead of calling the global abortTempAc(); keep the
existing behavior when this._abortSignal (config.abortSignal) is provided by
using that shared signal as-is, otherwise always create and own a fresh per-call
controller so overlapping mutations do not cancel each other (refer to
recreateTempAc, abortTempAc, mutationFn, onSettled, tempAc and this._abortSignal
to locate the changes).


Expand Down Expand Up @@ -386,7 +393,22 @@ export class Mutation<
this.mutationObserver.reset();
}

protected handleDestroy() {
protected abortTempAc() {
this.tempAc?.abort();
this.tempAc = undefined;
}

protected recreateTempAc() {
this.abortTempAc();
this.tempAc = new AbortController();
return this.tempAc;
}

destroy() {
super.destroy();

this.abortTempAc();

this._observerSubscription?.();

this.doneListeners = [];
Expand Down
3 changes: 2 additions & 1 deletion src/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,8 @@ export class Query<
this.hooks?.onQueryInit?.(this);
}

protected handleDestroy() {
destroy(): void {
super.destroy();
this.cleanup();
this.hooks?.onQueryDestroy?.(this);
}
Expand Down
28 changes: 13 additions & 15 deletions src/utils/destroyable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
import { LinkedAbortController } from 'linked-abort-controller';
import { action, makeObservable } from 'mobx';

export abstract class Destroyable implements Disposable {
protected abortController: LinkedAbortController;
protected _abortSignal?: AbortSignal;
protected _destroyed: boolean;

constructor(abortSignal?: AbortSignal) {
this.abortController = new LinkedAbortController(abortSignal);

action(this, 'handleDestroy');
makeObservable(this);

this.abortController.signal.addEventListener('abort', () => {
this.handleDestroy();
});
this._abortSignal = abortSignal;
this._destroyed = false;

this._abortSignal?.addEventListener(
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot May 6, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P2: Two lifecycle edge cases:

  1. If a pre-aborted signal is passed, addEventListener('abort', ...) will never fire (the event was already dispatched per DOM spec), so the instance is never destroyed automatically.
  2. No idempotency guard — if destroy() is called manually and the signal later aborts, the listener re-enters destroy(), causing subclass cleanup and hooks to fire twice.

Consider checking abortSignal?.aborted in the constructor (and deferring via queueMicrotask so subclass constructors finish), and adding an early if (this._destroyed) return; guard in destroy().

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/utils/destroyable.ts, line 9:

<comment>Two lifecycle edge cases:
1. If a pre-aborted signal is passed, `addEventListener('abort', ...)` will never fire (the event was already dispatched per DOM spec), so the instance is never destroyed automatically.
2. No idempotency guard — if `destroy()` is called manually and the signal later aborts, the listener re-enters `destroy()`, causing subclass cleanup and hooks to fire twice.

Consider checking `abortSignal?.aborted` in the constructor (and deferring via `queueMicrotask` so subclass constructors finish), and adding an early `if (this._destroyed) return;` guard in `destroy()`.</comment>

<file context>
@@ -1,26 +1,24 @@
+    this._abortSignal = abortSignal;
+    this._destroyed = false;
+
+    this._abortSignal?.addEventListener(
+      'abort',
+      () => {
</file context>
Fix with Cubic

'abort',
() => {
this.destroy();
},
{ once: true },
);
}

destroy() {
this.abortController?.abort();
this._destroyed = true;
}
Comment on lines 5 to 20
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 6, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Two lifecycle edge cases worth handling explicitly.

  1. Pre-aborted signal is silently ignored. Per the DOM spec, addEventListener('abort', ...) on an already-aborted AbortSignal does not fire. So if a consumer passes a signal that's already aborted to new Query({ abortSignal }), the instance is never marked _destroyed and never tears down.

  2. Double destroy() invocation. If destroy() is called manually (e.g. via using / Symbol.dispose, or a subclass's own teardown path) and the signal later aborts, the once: true listener still fires and re-enters destroy(). Subclasses (Query, InfiniteQuery, Mutation) re-run cleanup and re-fire onQueryDestroy / onInfiniteQueryDestroy / onMutationDestroy hooks.

🛡️ Proposed fix — guard idempotency and handle pre-aborted signals 
   constructor(abortSignal?: AbortSignal) {
     this._abortSignal = abortSignal;
     this._destroyed = false;

-    this._abortSignal?.addEventListener(
-      'abort',
-      () => {
-        this.destroy();
-      },
-      { once: true },
-    );
+    if (abortSignal?.aborted) {
+      // Defer so subclass constructors finish before destroy() runs.
+      queueMicrotask(() => this.destroy());
+    } else {
+      abortSignal?.addEventListener(
+        'abort',
+        () => this.destroy(),
+        { once: true },
+      );
+    }
   }

   destroy() {
+    if (this._destroyed) return;
     this._destroyed = true;
   }

The early-return in destroy() makes subclass overrides naturally idempotent (each one starts with super.destroy()).

📝 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
constructor(abortSignal?: AbortSignal) {
this.abortController = new LinkedAbortController(abortSignal);
action(this, 'handleDestroy');
makeObservable(this);
this.abortController.signal.addEventListener('abort', () => {
this.handleDestroy();
});
this._abortSignal = abortSignal;
this._destroyed = false;
this._abortSignal?.addEventListener(
'abort',
() => {
this.destroy();
},
{ once: true },
);
}
destroy() {
this.abortController?.abort();
this._destroyed = true;
}
constructor(abortSignal?: AbortSignal) {
this._abortSignal = abortSignal;
this._destroyed = false;
if (abortSignal?.aborted) {
// Defer so subclass constructors finish before destroy() runs.
queueMicrotask(() => this.destroy());
} else {
abortSignal?.addEventListener(
'abort',
() => this.destroy(),
{ once: true },
);
}
}
destroy() {
if (this._destroyed) return;
this._destroyed = true;
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/destroyable.ts` around lines 5 - 20, The constructor currently
ignores pre-aborted signals and destroy() is not idempotent; update constructor
(the class constructor that assigns this._abortSignal and this._destroyed) to
immediately call this.destroy() if this._abortSignal?.aborted is true, then
still add the abort listener as before, and change destroy() to be guarded (if
(this._destroyed) return) before setting this._destroyed = true so repeated
calls (from manual disposal, Symbol.dispose/using, or the abort listener) are
safe; reference the constructor, this._abortSignal, and destroy() so subclasses
like Query, InfiniteQuery, and Mutation rely on the idempotent super.destroy().


protected abstract handleDestroy(): void;

[Symbol.dispose](): void {
this.destroy();
}
Expand Down
Loading