Skip to content

fix(x-sdk): release XStream reader lock on early loop exit - #1970

Merged
kimteayon merged 5 commits into
mainfrom
fix/x-sdk
Jul 21, 2026
Merged

fix(x-sdk): release XStream reader lock on early loop exit#1970
kimteayon merged 5 commits into
mainfrom
fix/x-sdk

Conversation

@kimteayon

@kimteayon kimteayon commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

The asyncIterator acquired a reader with getReader() (locking the stream) but never released it, and had no try/finally. When a consumer broke, returned, or threw out of a for-await loop early, the reader's lock leaked and the stream could not be re-acquired or cancelled — the underlying piped stream stayed alive and buffered until it drained naturally.

Wrap the read loop in try/finally and call reader.releaseLock() in finally so the lock is released on both normal completion and early exit.

中文版模板 / Chinese template

🤔 This is a ...

  • 🆕 New feature
  • 🐞 Bug fix
  • 📝 Site / documentation improvement
  • 📽️ Demo improvement
  • 💄 Component style improvement
  • 🤖 TypeScript definition improvement
  • 📦 Bundle size optimization
  • ⚡️ Performance optimization
  • ⭐️ Feature enhancement
  • 🌐 Internationalization
  • 🛠 Refactoring
  • 🎨 Code style optimization
  • ✅ Test Case
  • 🔀 Branch merge
  • ⏩ Workflow
  • ⌨️ Accessibility improvement
  • ❓ Other (about what?)

🔗 Related Issues

  • Describe the source of related requirements, such as links to relevant issue discussions.
  • For example: close #xxxx, fix #xxxx

💡 Background and Solution

  • The specific problem to be addressed.
  • List the final API implementation and usage if needed.
  • If there are UI/interaction changes, consider providing screenshots or GIFs.

📝 Change Log

Language Changelog
🇺🇸 English
🇨🇳 Chinese

Summary by CodeRabbit

  • Bug Fixes
    • 修复了在 for await...of 中提前 break/return/throw 时,XStream 读取锁可能未释放导致后续无法正确取消或重新获取的问题。
    • 现在可区分正常结束与中途终止,确保读取完成/取消后始终释放锁并及时停止底层读取。
  • Tests
    • 新增用例:提前退出后断言 locked 恢复,并验证底层读取已结束(读取返回 done === true),同时完成锁释放。

The asyncIterator acquired a reader with getReader() (locking the stream)
but never released it, and had no try/finally. When a consumer broke,
returned, or threw out of a for-await loop early, the reader's lock leaked
and the stream could not be re-acquired or cancelled — the underlying
piped stream stayed alive and buffered until it drained naturally.

Wrap the read loop in try/finally and call reader.releaseLock() in
finally so the lock is released on both normal completion and early exit.

Co-Authored-By: Claude <noreply@anthropic.com>
@dosubot dosubot Bot added bug Something isn't working javascript Pull requests that update Javascript code labels Jul 9, 2026
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 41093948-69fb-4ac1-8430-e66321dd3a19

📥 Commits

Reviewing files that changed from the base of the PR and between 5f97acd and 0018497.

📒 Files selected for processing (2)
  • packages/x-sdk/src/x-stream/__test__/index.test.ts
  • packages/x-sdk/src/x-stream/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/x-sdk/src/x-stream/test/index.test.ts
  • packages/x-sdk/src/x-stream/index.ts

📝 Walkthrough

Walkthrough

修改了 XStream 异步迭代器的退出处理:正常结束时标记完成,提前退出时取消底层读取,并始终释放 reader 锁。新增测试验证提前 break 后锁已释放,且后续读取返回结束状态。

Changes

Reader 锁释放修复

Layer / File(s) Summary
异步迭代器清理与测试
packages/x-sdk/src/x-stream/index.ts, packages/x-sdk/src/x-stream/__test__/index.test.ts
asyncIterator 改为 try...finally,提前终止时调用 reader.cancel(),无论何种结束都调用 reader.releaseLock();新增测试覆盖提前 break 后的锁状态和底层流结束状态。

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Consumer
  participant XStreamIterator
  participant Reader

  Consumer->>XStreamIterator: for await...of 消费数据
  XStreamIterator->>Reader: reader.read()
  Reader-->>XStreamIterator: { done, value }
  alt done 为 true
    XStreamIterator->>XStreamIterator: completed = true
  else 继续产出
    XStreamIterator-->>Consumer: yield value
  end
  Consumer->>XStreamIterator: break(提前退出)
  XStreamIterator->>Reader: reader.cancel()
  XStreamIterator->>Reader: reader.releaseLock()
Loading

Poem

小兔读流轻轻跳,
提前退出也不闹;
finally 里锁放好,
流停下来真可靠。
🐰

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题准确概括了本次修复的核心:在 XStream 早退时释放 reader lock。
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/x-sdk

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bundle Report

Changes will increase total bundle size by 82 bytes (0.0%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
antdx-array-push 2.11MB 82 bytes (0.0%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: antdx-array-push

Assets Changed:

Asset Name Size Change Total Size Change (%)
antdx.min.js 82 bytes 2.11MB 0.0%

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request wraps the custom async iterator of XStream in a try...finally block to ensure the reader lock is released when a consumer exits the loop early, and adds a corresponding test. The reviewer points out that simply releasing the lock does not cancel the underlying stream, which can lead to resource leaks (e.g., active network requests continuing in the background). The reviewer suggests tracking whether the iteration completed and calling reader.cancel() in the finally block if it was exited early, aligning the behavior with the standard ReadableStream async iterator.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +210 to 225
try {
while (true) {
const { done, value } = await reader.read();

if (done) break;
if (done) break;

if (!value) continue;
if (!value) continue;

// Transformed data through all transform pipes
yield value;
// Transformed data through all transform pipes
yield value;
}
} finally {
// Always release the lock so the stream can be re-acquired or cancelled,
// even when the consumer breaks/returns/throws out of the loop early.
reader.releaseLock();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

While releasing the reader lock on early exit is a great fix, simply releasing the lock does not cancel the underlying stream.

According to the Streams Standard, the default async iterator of a ReadableStream automatically cancels the stream when the loop is exited early.

Since XStream overrides Symbol.asyncIterator with a custom implementation, if a consumer breaks, returns, or throws out of the for await loop early, the stream will remain active and continue fetching/buffering data in the background (e.g., an active network request for LLM token generation will keep running, wasting server resources and bandwidth).

To align with standard ReadableStream behavior and prevent resource leaks, we should also cancel the reader in the finally block if the stream iteration did not complete normally.

    let completed = false;

    try {
      while (true) {
        const { done, value } = await reader.read();

        if (done) {
          completed = true;
          break;
        }

        if (!value) continue;

        // Transformed data through all transform pipes
        yield value;
      }
    } finally {
      if (!completed) {
        reader.cancel().catch(() => {});
      }
      // Always release the lock so the stream can be re-acquired or cancelled,
      // even when the consumer breaks/returns/throws out of the loop early.
      reader.releaseLock();
    }

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 9, 2026

Copy link
Copy Markdown

Deploying ant-design-x with  Cloudflare Pages  Cloudflare Pages

Latest commit: ec16d78
Status: ✅  Deploy successful!
Preview URL: https://4f6c699c.ant-design-x.pages.dev
Branch Preview URL: https://fix-x-sdk.ant-design-x.pages.dev

View logs

@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.06%. Comparing base (be739c5) to head (ec16d78).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1970   +/-   ##
=======================================
  Coverage   97.06%   97.06%           
=======================================
  Files         159      159           
  Lines        5755     5762    +7     
  Branches     1684     1714   +30     
=======================================
+ Hits         5586     5593    +7     
  Misses        167      167           
  Partials        2        2           

☔ View full report in Codecov by Harness.
📢 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.

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

size-limit report 📦

Path Size
packages/x/dist/antdx.min.js 510.92 KB (+106 B 🔺)
packages/x-sdk/dist/x-sdk.min.js 55.53 KB (+68 B 🔺)
packages/x-markdown/dist/x-markdown.min.js 34.42 KB
packages/x-markdown/dist/plugins/latex.min.js 61.91 KB

fanlu.lby and others added 3 commits July 9, 2026 22:45
The custom Symbol.asyncIterator only released the reader lock on early
exit (break/return/throw), which unlike the default ReadableStream
async iterator left the underlying stream active — so an in-flight fetch
(e.g. LLM token generation) kept buffering data in the background.

Track a 'completed' flag and cancel the reader in the finally block when
the loop did not finish normally, aligning with standard ReadableStream
semantics and preventing server/bandwidth waste. cancel() runs before
releaseLock() (needs the lock) and swallows rejection via catch.

Co-Authored-By: Claude <noreply@anthropic.com>
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jul 21, 2026
@kimteayon
kimteayon merged commit 028b743 into main Jul 21, 2026
15 checks passed
@kimteayon
kimteayon deleted the fix/x-sdk branch July 21, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working javascript Pull requests that update Javascript code lgtm This PR has been approved by a maintainer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants