Ingress: Stop streaming response if stream is EOF #5985
Conversation
An empty data chunk signifies EOF. Stop streaming the response in that case. See https://github.com/aio-libs/aiohttp/blob/v3.12.13/aiohttp/streams.py#L471-L474.
📝 WalkthroughWalkthroughThe update modifies the streaming logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IngressAPI
participant ProxiedService
Client->>IngressAPI: Send request
IngressAPI->>ProxiedService: Forward request
ProxiedService-->>IngressAPI: Stream response chunks
loop For each chunk
IngressAPI->>IngressAPI: Check if chunk is empty
alt Chunk is not empty
IngressAPI-->>Client: Write chunk
else Chunk is empty
IngressAPI->>IngressAPI: Break loop, stop streaming
end
end
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (3.3.7)supervisor/api/ingress.py✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
supervisor/api/ingress.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.5
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
There was a problem hiding this comment.
Pull Request Overview
Stop streaming the response on receiving an empty data chunk (EOF) to prevent sending unnecessary empty writes.
- Add an EOF check inside the streaming loop.
- Break out of the loop when no data is returned.
Comments suppressed due to low confidence (2)
supervisor/api/ingress.py:283
- Consider adding a test (unit or integration) to cover the case where
iter_chunks()yields an empty data chunk, verifying that the loop breaks as expected and the response completes cleanly.
if not data:
supervisor/api/ingress.py:283
- [nitpick] Add an inline comment explaining that an empty
datachunk signifies EOF per aiohttp semantics, and that the break ensures the stream terminates properly.
if not data:
|
This needs more discussion, see home-assistant/core#147899 (comment). |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Core code adopted the approach avoid reading empty content by checking the request using No changes needed on Supervisor side. A fix which avoids the endless loop in aiohttp got proposed with aio-libs/aiohttp#11397. |
1 participant
Proposed change
An empty data chunk signifies EOF. Stop streaming the response in that case.
See https://github.com/aio-libs/aiohttp/blob/v3.12.13/aiohttp/streams.py#L471-L474.
Related to: home-assistant/core#147899
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit