set of fixes/improvements for http proxy #17998
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces various improvements and fixes for the HTTP proxy module, addressing issues such as hanging stored connections after timeout expiry, built-in header corrections, and simplifying the request syntax while extending static content handling.
- Added an overload for the HTTP static content handler using a URL adapter (lambda) to process URL paths.
- Refactored the outgoing connection actor to integrate the TEvHttpOutgoingRequest event and updated error message handling.
- Modified header handling in HTTP responses to check for preexisting worker names before setting them.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ydb/library/actors/http/http_static.h & http_static.cpp | Introduced a URL adapter-based overload for the static content handler and adjusted MIME type derivation logic. |
| ydb/library/actors/http/http_proxy_outgoing.cpp | Refactored the connection actor's construction, error reporting, and added a new CheckClose loop for connection reuse handling. |
| ydb/library/actors/http/http_proxy.h & http_proxy.cpp | Updated the outgoing connection actor creation signature and modified event forwarding logic. |
| ydb/library/actors/http/http.h & http.cpp | Added an initializer_list constructor to THeadersBuilder and enhanced header checks for the worker name. |
Comments suppressed due to low confidence (3)
ydb/library/actors/http/http_proxy_outgoing.cpp:156
- [nitpick] Returning an empty error message when 'res' equals 0 could obscure the reason for connection closure; consider logging an explicit message to aid debugging.
ReplyErrorAndPassAway(res == 0 ? "" : strerror(-res));
ydb/library/actors/http/http_static.cpp:56
- Ensure that using GetName() for MIME type determination correctly extracts the file extension; consider adding a comment to clarify if this behavior is a deliberate improvement over GetExtension().
TString contentType = mimetypeByExt(url.GetName().c_str());
ydb/library/actors/http/http_proxy.cpp:113
- Verify that the new Forward function preserves all necessary event metadata (e.g., TraceId) to maintain proper event tracing and debugging.
Send(Forward(availableConnection, std::move(event)));
adameat
added
area/ui
|
🟢 |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
|
⚪ Test history | Ya make output | Test bloat
⚪ Test history | Ya make output | Test bloat | Test bloat
🟢
*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation |
There was a problem hiding this comment.
Pull Request Overview
This PR implements several improvements to enhance the HTTP proxy functionality such as addressing hanging connections, improving header handling, streamlining the request syntax, and extending support for static content.
- Fixes hanging stored connections after timeouts expire
- Corrects built-in header handling and simplifies request syntax
- Extends and refactors the static content handler API
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ydb/library/actors/http/http_ut.cpp | Adjusts assertion ordering for error responses in tests. |
| ydb/library/actors/http/http_static.h | Introduces an overload for static content handler with a URL adapter. |
| ydb/library/actors/http/http_static.cpp | Refactors static content handler to replace the index member with a URL adapter and changes mime type determination. |
| ydb/library/actors/http/http_proxy_outgoing.cpp | Refactors the outgoing connection actor to use bootstrapping and streamlines the request initialization and error handling. |
| ydb/library/actors/http/http_proxy.h | Updates outgoing connection actor API signature. |
| ydb/library/actors/http/http_proxy.cpp | Adapts outgoing connection creation and uses a forwarding helper for event handling. |
| ydb/library/actors/http/http.h | Adds an initializer_list overload for THeadersBuilder. |
| ydb/library/actors/http/http.cpp | Adjusts header injection logic in responses and provides the new THeadersBuilder constructor. |
Comments suppressed due to low confidence (2)
ydb/library/actors/http/http_static.cpp:56
- Using GetName() instead of GetExtension() for mime type determination could result in incorrect content type detection if the file name does not contain the expected extension. Consider reverting to GetExtension() or providing additional logic to extract the file extension reliably.
TString contentType = mimetypeByExt(url.GetName().c_str());
ydb/library/actors/http/http_proxy_outgoing.cpp:160
- [nitpick] Changing the error message from 'ConnectionClosed' to an empty string for a zero result may be inconsistent with unit tests that expect 'ConnectionClosed'. Verify that the conditional logic in ReplyErrorAndPassAway correctly handles the case when no error string is provided.
ReplyErrorAndPassAway(res == 0 ? "" : strerror(-res));
Changelog entry
...
Changelog category
Description for reviewers
fixes hanging stored connection after timeout expires.
fixes builtin headers
simplifies request syntax
extends static handler