fix: crash when KomodoDefiSdk is disposed during periodic fetch#3117
fix: crash when KomodoDefiSdk is disposed during periodic fetch#3117
Conversation
Added a `_closed` flag to TradingEntitiesBloc to prevent periodic updates from running after the SDK has been disposed. The timer callback and `fetch` now check `_closed` before accessing `_kdfSdk`. This prevents `StateError (Bad state: KomodoDefiSdk has been disposed)` from crashing the app after sign-out or SDK disposal.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughIntroduces a private _closed flag to TradingEntitiesBloc, adds early-return guards in fetch() and the Timer.periodic loop, wraps fetch logic in try/catch/finally within runUpdate, sets _closed on StateError containing 'disposed', logs other errors, and ensures updateInProgress is reset in finally. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Timer.periodic
participant B as TradingEntitiesBloc
participant F as fetch()
T->>B: tick()
alt B._closed == true
B-->>T: return (stop work)
else Not closed
B->>B: set updateInProgress = true
B->>F: fetch()
alt fetch succeeds
F-->>B: data
B->>B: finally: updateInProgress = false
else fetch throws
F-->>B: error
alt StateError contains "disposed"
B->>B: _closed = true
else Other error
B->>B: log "TradingEntitiesBloc.fetch"
end
B->>B: finally: updateInProgress = false
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Visit the preview URL for this PR (updated for commit 8bc1e85): https://walletrc--pull-3117-merge-j3jo4mw1.web.app (expires Thu, 18 Sep 2025 14:08:58 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f66a4ff03faa546f12f0ae5a841bd9eff2714dcc |
|
@cursor review @coderabbitai review |
|
@CharlVS I'll review the changes in this PR for you. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash that occurs when the KomodoDefiSdk is disposed while a periodic timer is still attempting to fetch trading data. The fix adds a _closed flag to prevent operations after disposal.
- Added a
_closedboolean flag to track when the bloc should stop operations - Enhanced the periodic timer callback with proper error handling and disposal detection
- Added early return checks in both
fetch()and the timer callback to prevent accessing disposed SDK
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| updateInProgress = true; | ||
| await fetch(); | ||
| updateInProgress = false; | ||
| try { | ||
| await fetch(); | ||
| } catch (e) { | ||
| if (e is StateError && e.message.contains('disposed')) { |
There was a problem hiding this comment.
The error message check is fragile and could miss disposal errors with different wording. Consider checking the error type more specifically or using a more robust method to detect SDK disposal state.
| updateInProgress = true; | |
| await fetch(); | |
| updateInProgress = false; | |
| try { | |
| await fetch(); | |
| } catch (e) { | |
| if (e is StateError && e.message.contains('disposed')) { | |
| // Check SDK disposal state before running fetch | |
| if (_kdfSdk is Disposable && (_kdfSdk as dynamic).isDisposed == true) { | |
| _closed = true; | |
| return; | |
| } | |
| updateInProgress = true; | |
| try { | |
| await fetch(); | |
| } catch (e) { | |
| if (e is StateError) { |
| bool _closed = false; | ||
|
|
There was a problem hiding this comment.
The _closed flag lacks a proper disposal mechanism. Consider adding a dispose() method that sets _closed = true and cancels the timer to ensure clean resource cleanup.
| bool _closed = false; | |
| /// Dispose resources and mark bloc as closed. | |
| void dispose() { | |
| _closed = true; | |
| timer?.cancel(); | |
| _authModeListener?.cancel(); | |
| _myOrdersController.close(); | |
| _swapsController.close(); | |
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/blocs/trading_entities_bloc.dart (1)
169-172: Await order cancellations; current implementation returns early and can drop errors
Withoutawait, the cancellations run unobserved and exceptions are lost.Future<void> cancelAllOrders() async { final futures = myOrders.map((o) => cancelOrder(o.uuid)); - Future.wait(futures); + await Future.wait(futures); }
🧹 Nitpick comments (7)
lib/blocs/trading_entities_bloc.dart (7)
86-99: Log stack trace and correct log context; keep finally semantics
Include the stack trace for diagnosis and tag the log with the caller context (runUpdate).- } catch (e) { + } catch (e, st) { if (e is StateError && e.message.contains('disposed')) { _closed = true; timer?.cancel(); timer = null; return; } else { - await log( - 'fetch error: $e', - path: 'TradingEntitiesBloc.fetch', - ); + await log( + 'fetch error: $e\n$st', + path: 'TradingEntitiesBloc.runUpdate', + ); } } finally { updateInProgress = false; }
65-70: Guard is good; optionally handle disposed directly in fetch()
This prevents one extra throw path after disposal whenisSignedIn()touches the disposed SDK. Optional duplication of the catch, but keepsfetch()safe if called outsiderunUpdate().Future<void> fetch() async { if (_closed) return; - if (!await _kdfSdk.auth.isSignedIn()) return; - - myOrders = await _myOrdersService.getOrders() ?? []; - swaps = await getRecentSwaps(MyRecentSwapsRequest()) ?? []; + try { + if (!await _kdfSdk.auth.isSignedIn()) return; + myOrders = await _myOrdersService.getOrders() ?? []; + swaps = await getRecentSwaps(MyRecentSwapsRequest()) ?? []; + } on StateError catch (e) { + if (e.message.contains('disposed')) { + _closed = true; + timer?.cancel(); + timer = null; + return; + } + rethrow; + } }
109-120: Avoid materializing lists; short-circuit with any()
Reduces allocations and improves readability.bool isCoinBusy(String coin) { - return (_swaps - .where((swap) => !swap.isCompleted) - .where((swap) => swap.sellCoin == coin || swap.buyCoin == coin) - .toList() - .length + - _myOrders - .where((order) => order.base == coin || order.rel == coin) - .toList() - .length) > - 0; + final hasActiveSwap = _swaps.any( + (s) => !s.isCompleted && (s.sellCoin == coin || s.buyCoin == coin), + ); + if (hasActiveSwap) return true; + return _myOrders.any((o) => o.base == coin || o.rel == coin); }
159-167: Protect against divide-by-zero in progress calculation
Iforder.baseAmountis zero, the UI may getInfinity/NaN.double getProgressFillSwap(MyOrder order) { @@ - return swapFill / order.baseAmount.toDouble(); + final base = order.baseAmount.toDouble(); + if (base == 0) return 0; + return swapFill / base; }
41-50: Do not mutate caller-provided lists in setters
Clone before sort to avoid side-effects on the input list.set myOrders(List<MyOrder> orderList) { - orderList.sort((first, second) => second.createdAt - first.createdAt); - _myOrders = orderList; + final sorted = + [...orderList]..sort((a, b) => b.createdAt - a.createdAt); + _myOrders = sorted; _inMyOrders.add(_myOrders); }
57-62: Same cloning suggestion for swaps setter
Avoids mutatingswapListpassed by callers.set swaps(List<Swap> swapList) { - swapList.sort((first, second) => - (second.myInfo?.startedAt ?? 0) - (first.myInfo?.startedAt ?? 0)); - _swaps = swapList; + final sorted = [...swapList] + ..sort((a, b) => (b.myInfo?.startedAt ?? 0) - (a.myInfo?.startedAt ?? 0)); + _swaps = sorted; _inSwaps.add(_swaps); }
39-39: Nit: consider renaming to_isClosed
Improves readability at call sites (if (_isClosed) return;).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lib/blocs/trading_entities_bloc.dart(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Cursor Bugbot
- GitHub Check: Cursor Bugbot
- GitHub Check: Build Desktop (linux)
- GitHub Check: Build Desktop (windows)
- GitHub Check: Build Desktop (macos)
- GitHub Check: build_and_preview
- GitHub Check: validate_code_guidelines
- GitHub Check: Test web-app-linux-profile
- GitHub Check: unit_tests
- GitHub Check: Test web-app-macos
- GitHub Check: Build Mobile (Android)
- GitHub Check: Build Mobile (iOS)
| List<MyOrder> _myOrders = []; | ||
| List<Swap> _swaps = []; | ||
| Timer? timer; | ||
| bool _closed = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stop the timer and mark closed during disposal to prevent leaks and stray ticks
Right now the periodic timer keeps firing after close and holds references. Cancel it when _closed flips to true and in dispose(). Also mark _closed in dispose() and close the controllers.
Timer? timer;
bool _closed = false;
@@
@override
void dispose() {
- _authModeListener?.cancel();
+ _closed = true;
+ timer?.cancel();
+ timer = null;
+ _authModeListener?.cancel();
+ // Best-effort; returns Future<void>, but dispose() is sync.
+ _myOrdersController.close();
+ _swapsController.close();
}
@@
timer = Timer.periodic(const Duration(seconds: 1), (_) async {
- if (_closed) return;
+ if (_closed) {
+ timer?.cancel();
+ timer = null;
+ return;
+ }
@@
- } catch (e) {
- if (e is StateError && e.message.contains('disposed')) {
- _closed = true;
- } else {
+ } catch (e) {
+ if (e is StateError && e.message.contains('disposed')) {
+ _closed = true;
+ timer?.cancel();
+ timer = null;
+ return;
+ } else {
await log(
'fetch error: $e',
path: 'TradingEntitiesBloc.fetch',
);
}
} finally {Also applies to: 72-75, 81-81, 89-91
🤖 Prompt for AI Agents
In lib/blocs/trading_entities_bloc.dart around line 39 (and similarly lines
72-75, 81, 89-91), the periodic Timer is never cancelled and _closed is never
set during disposal; update the class so that when you set _closed = true you
also cancel the Timer (if not null and active), and in dispose() set _closed =
true, cancel the Timer, and close any StreamControllers/subjects used by the
bloc; ensure any periodic callbacks check _closed before running and that all
controller.close() calls are invoked in dispose to prevent leaks and stray
ticks.
CharlVS
changed the title
CharlVS
left a comment
CharlVS
left a comment
There was a problem hiding this comment.
As discussed in DMs, you said the AI review comments do not apply to the scope of this PR and/or the comments do not need to be addressed.
Added a
_closedflag to TradingEntitiesBloc to prevent periodic updates from running after the SDK has been disposed. The timer callback andfetchnow check_closedbefore accessing_kdfSdk.This prevents
StateError (Bad state: KomodoDefiSdk has been disposed)from crashing the app after sign-out or SDK disposal.On Linux, after exit we saw an exception and crash like this:
This PR should fix this behavior.
TODO: maybe we should also consider
timer?.cancel()logic here.Summary by CodeRabbit