Fix hang when closing connections under load - #2575
Fix hang when closing connections under load#2575Nicolae Vartolomei (nvartolomei) wants to merge 1 commit into
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
c3b8e4f to
0551c02
Compare
This comment was marked as resolved.
This comment was marked as resolved.
0551c02 to
70d0d06
Compare
|
Nicolae Vartolomei (@nvartolomei) xiaonlimsft (@XiaoningLiu) , EmmaZhu-MSFT (@EmmaZhu), |
70d0d06 to
7382c5c
Compare
…erationQueue processes the request In Azurite, all operations are managed through concurrent operation queues. A bug was identified where operations could hang indefinitely if the client disconnected before the operation was processed. This occurred because Azurite would attempt to attach event handlers to the request's readable stream (body) after it had already been closed by the client's disconnection. Since a closed stream emits no further events (like 'data', 'close', or 'error'), the operation would never complete, causing a permanent hang. This fix addresses the issue by checking if the request stream is still readable before attaching any event handlers. This ensures that we only process requests that are still active, preventing the hang and allowing the queues to continue processing other operations.
7382c5c to
fe3b28e
Compare
|
EmmaZhu-MSFT (@EmmaZhu) would be great to merge this! |
|
Wei Wei (@blueww) mind taking another look at merging this? Anything blocking it? |
|
Akanksha Jain (@jainakanksha-msft) Would you please help to review and approve if no issue found? |
|
Nicolae Vartolomei (@nvartolomei), could you please refresh the PR from main and address copilot comments if any |
There was a problem hiding this comment.
Pull request overview
Fixes an indefinite hang in Azurite’s blob persistence write path by failing fast when an incoming request/body stream is already non-readable (e.g., client disconnected before the operation queue begins processing), and adds regression coverage plus a changelog entry.
Changes:
- Add a readability guard in
FSExtentStore.streamPipe()to reject immediately when the readable stream is already closed/destroyed. - Add a test that simulates an aborted/destroyed input stream to ensure
appendExtent()does not hang. - Document the fix in the upcoming release changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/blob/fsStore.test.ts | Adds a regression test for destroyed/aborted input streams during appendExtent(). |
| src/common/persistence/FSExtentStore.ts | Rejects early in streamPipe() when the input stream is not readable to prevent hanging operations. |
| ChangeLog.md | Adds an “Upcoming Release” entry describing the hang fix. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| try { | ||
| await store.appendExtent(stream1); | ||
| assert.fail("Expected an error to be thrown due to destroyed stream"); | ||
| } catch (err) { | ||
| assert.deepStrictEqual(err.message, "FSExtentStore:streamPipe() Readable stream is not readable."); | ||
| } |
| reject( | ||
| new Error( | ||
| `FSExtentStore:streamPipe() Readable stream is not readable.` | ||
| )); |
| assert.strictEqual(await readIntoString(readable3), "Test"); | ||
| }); | ||
|
|
||
| it("should handle garbage collected input stream during appendExtent @loki", async () => { |
Thanks for contribution! Please go through following checklist before sending PR.
PR Branch Destination
mainbranch.legacy-devbranch.Always Add Test Cases
Make sure test cases are added to cover the code change.
Add Change Log
Add change log for the code change in
Upcoming Releasesection inChangeLog.md.Development Guideline
Please go to CONTRIBUTION.md for steps about setting up development environment and recommended Visual Studio Code extensions.