Repository navigation
Conversation
Per-key tables from PartitionedTable.getTable are client-owned exports; neither the widget close nor PartitionedTable.close() releases them. Close them in close(), and release key tables or the partitioned table that resolve after close() instead of registering them.
|
tradingview-lightweight docs preview (Available for 14 days) |
A key table can still have a snapshot in flight on a superseded subscription (e.g. a direct fallback after a failed downsample, retired by the next successful one). Release key tables with the widget once every retirement drains instead of at close(), and skip the downsample failure fallback when the model is already closed.
|
tradingview-lightweight docs preview (Available for 14 days) |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A late partition fetch can reattach widget listeners after the model has closed.
1 open finding
What changed in this PR
Fixes server-side export leaks from partition-key tables in TradingView by= charts.
Changes:
- Tracks and safely releases partition-key tables after subscriptions drain.
- Handles asynchronous results arriving after closure.
- Adds lifecycle regression tests.
| File | Description |
|---|---|
TradingViewChartModel.ts |
Adds partition-table lifecycle management. |
TradingViewChartModel.test.ts |
Tests closure and asynchronous race scenarios. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…during init If close() runs during one of init()'s awaits, it has already done its listener cleanup, so attaching widget listeners afterwards leaves them attached for good.
dsmmcken
marked this pull request as ready for review
October 13, 2026 16:01
|
tradingview-lightweight docs preview (Available for 14 days) |
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
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.
For a
by=chart, the client fetches one table per partition key withPartitionedTable.getTable(key). Those tables are client-owned exports: closing the widget or thePartitionedTabledoesn't release them. Each time aby=chart was closed or reconnected, its per-key tables stayed open on the server until the session ended.Changes in
TradingViewChartModel:getTableor thePartitionedTablefetch resolves afterclose(), release the result instead of registering it. Previously both leaked, and the key table was also subscribed after close.subscribeTabledoes nothing afterclose(), so an async fallback can't open a subscription that is never retired.Unit tests cover each case.
Found while checking that the
by=performance issues fixed in #1428 for plotly-express don't show up in TVL. They don't, but this leak turned up along the way.