fix(polling): drain edge-triggered socket paths#489
Closed
stackia wants to merge 6 commits into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens edge-triggered polling paths so handlers that consume bytes still report progress and continue draining sockets, and adds a deterministic 400 response for oversized incomplete HTTP requests.
Changes:
- Refactors
rtsp_handle_socket_eventto loop overrtsp_try_receive_response, introducesRTSP_RESPONSE_PROGRESS, and routes hangups through a newrtsp_handle_terminal_socket_eventhelper that runs only after readable data is drained. - Adds
drain_socket_until_eagainhelper and uses it from FCC, multicast, and RTSP UDP buffer-exhaustion drop paths; also continues UDP RTP draining after STUN packets. - HTTP proxy header-phase recv returns bytes consumed to keep the outer drain loop going; connection read-loop sends a 400 when the request fills the input buffer without completing headers; new e2e tests cover oversized request line/headers.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils.h | Declares drain_socket_until_eagain. |
| src/utils.c | Implements drain helper that loops recv until EAGAIN/EWOULDBLOCK. |
| src/rtsp.c | Restructures terminal-event handling, drain loop, progress signalling, and STUN/RTP continuation. |
| src/multicast.c | Switches buffer-exhaustion drop drain to the new helper. |
| src/http_proxy.c | Header-phase recv returns bytes consumed for edge-triggered draining. |
| src/fcc.c | Uses the new drain helper on buffer-exhaustion drop path. |
| src/connection.c | Sends 400 when HTTP request exceeds INBUF_SIZE before headers complete. |
| e2e/test_error.py | Adds raw-socket tests for oversized incomplete request line/headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://thankful-water-0a297bf00-489.eastasia.1.azurestaticapps.net |
Contributor
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://thankful-water-0a297bf00-489.eastasia.1.azurestaticapps.net |
Comment on lines
808
to
+955
| @@ -891,6 +891,7 @@ static int http_proxy_try_receive_response(http_proxy_session_t *session) { | |||
| } | |||
|
|
|||
| session->response_buffer_pos += received; | |||
| int socket_progress = (int)received; | |||
|
|
|||
| /* Try to parse headers */ | |||
| if (!session->headers_received) { | |||
| @@ -899,7 +900,7 @@ static int http_proxy_try_receive_response(http_proxy_session_t *session) { | |||
| return -1; | |||
| } | |||
| if (result == 0) { | |||
| return 0; /* Need more data for headers */ | |||
| return socket_progress; /* Progress: keep draining edge-triggered sockets */ | |||
| } | |||
| /* result > 0 means headers complete, state is now STREAMING */ | |||
| } | |||
| @@ -930,6 +931,8 @@ static int http_proxy_try_receive_response(http_proxy_session_t *session) { | |||
| logger(LOG_DEBUG, "HTTP Proxy: All M3U content received with headers (%zd bytes)", session->bytes_received); | |||
| return http_proxy_finalize_rewrite(session); | |||
| } | |||
|
|
|||
| bytes_forwarded = (int)initial_size; | |||
| } else { | |||
| /* Normal mode: forward immediately */ | |||
| if (connection_queue_output(session->conn, session->response_buffer, session->response_buffer_pos) < 0) { | |||
| @@ -949,7 +952,7 @@ static int http_proxy_try_receive_response(http_proxy_session_t *session) { | |||
| } | |||
| } | |||
|
|
|||
| return bytes_forwarded; | |||
| return bytes_forwarded > 0 ? bytes_forwarded : socket_progress; | |||
| uint8_t dummy[BUFFER_POOL_BUFFER_SIZE]; | ||
| recv(session->sock, dummy, sizeof(dummy), 0); | ||
| if (drain_datagram_socket_until_eagain(session->sock) < 0) { | ||
| logger(LOG_DEBUG, "Multicast: Drain failed: %s", strerror(errno)); |
Comment on lines
+151
to
+169
| int drain_datagram_socket_until_eagain(int fd) { | ||
| uint8_t dummy[2048]; | ||
| int reads = 0; | ||
|
|
||
| for (;;) { | ||
| ssize_t nread = recv(fd, dummy, sizeof(dummy), 0); | ||
| if (nread >= 0) { | ||
| reads++; | ||
| continue; | ||
| } | ||
| if (errno == EINTR) { | ||
| continue; | ||
| } | ||
| if (errno == EAGAIN || errno == EWOULDBLOCK) { | ||
| return reads; | ||
| } | ||
| return -1; | ||
| } | ||
| } |
Comment on lines
+1922
to
+1930
| int drained = drain_datagram_socket_until_eagain(session->rtp_socket); | ||
| if (drained >= 0) { | ||
| /* The drain count includes the first queued datagram that could not | ||
| * get a buffer plus any additional datagrams dropped behind it. */ | ||
| session->packets_dropped += (uint64_t)drained; | ||
| } else { | ||
| session->packets_dropped++; | ||
| logger(LOG_ERROR, "RTSP: RTP drain failed: %s", strerror(errno)); | ||
| } |
Comment on lines
+715
to
+716
| logger(LOG_WARN, "HTTP request exceeded input buffer before headers completed"); | ||
| http_send_400(c); |
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.
Summary
This fixes the remaining edge-triggered polling audit follow-ups on top of the HTTP proxy M3U drain branch.
The change makes socket progress reporting consistent when bytes are consumed but protocol messages are still incomplete, drains RTSP readable data before hangup handling, continues RTP UDP draining after STUN transition packets, and drains UDP sockets fully when dropping packets under buffer-pool pressure. It also adds a deterministic guard for oversized incomplete client HTTP requests.
Root Cause
Several handlers consumed data from edge-triggered sockets but returned a no-progress result, or exited early after a state transition. With EPOLLET and EV_CLEAR, unread bytes already queued in the kernel may not trigger another readiness event, so those paths could stall until timeout or another packet arrived.
Validation
git diff --check HEAD~1..HEADcmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_AGGRESSIVE_OPT=ONcmake --build build -j$(getconf _NPROCESSORS_ONLN)./scripts/run-e2e.sh(432 passed, 8 skipped)