Repository navigation
rest: replace select() polling with event-driven CurlIoLoop and support HTTP/2 - #1956
AustinBenoit wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the legacy uWebSockets and libuv WebSocket transport with a libcurl-based implementation on desktop platforms, consolidating all REST and Realtime Database WebSocket traffic onto a single shared, event-driven I/O thread. It also centralizes the TLS policy configuration and removes the libuv and uWebSockets dependencies. The review feedback highlights potential unsigned integer underflow risks in web_socket_client_impl.cc when performing bounds checks on incoming PING and message payloads, recommending that these subtraction-based checks be rewritten using addition.
| if ((meta->flags & CURLWS_CONT) != 0 || | ||
| rlen > kMaxPingPayloadBytes - incoming_ping_buffer_.size() || | ||
| frame_remaining > | ||
| kMaxPingPayloadBytes - (incoming_ping_buffer_.size() + rlen)) { |
There was a problem hiding this comment.
To prevent potential unsigned integer underflow when checking the bounds of the incoming PING frame payload, rewrite the subtraction checks using addition. This is a safer, more defensive programming practice.
| if ((meta->flags & CURLWS_CONT) != 0 || | |
| rlen > kMaxPingPayloadBytes - incoming_ping_buffer_.size() || | |
| frame_remaining > | |
| kMaxPingPayloadBytes - (incoming_ping_buffer_.size() + rlen)) { | |
| if ((meta->flags & CURLWS_CONT) != 0 || | |
| incoming_ping_buffer_.size() + rlen > kMaxPingPayloadBytes || | |
| incoming_ping_buffer_.size() + rlen + frame_remaining > | |
| kMaxPingPayloadBytes) { |
There was a problem hiding this comment.
Note: incoming_ping_buffer_.size() <= kMaxPingPayloadBytes is an invariant enforced before every append(), so kMaxPingPayloadBytes - incoming_ping_buffer_.size() cannot underflow, whereas incoming_ping_buffer_.size() + rlen + frame_remaining could overflow uint64_t if a peer sent a header with bytesleft near INT64_MAX. Also note database/ now lives in base PR #1959 (0 lines touched in #1956).
| if (rlen > kMaxMessagePayloadBytes - incoming_message_buffer_.size() || | ||
| frame_remaining > | ||
| kMaxMessagePayloadBytes - | ||
| (incoming_message_buffer_.size() + rlen)) { |
There was a problem hiding this comment.
Similarly, to prevent potential unsigned integer underflow when checking the bounds of the incoming message payload, rewrite the subtraction checks using addition.
if (incoming_message_buffer_.size() + rlen > kMaxMessagePayloadBytes ||
incoming_message_buffer_.size() + rlen + frame_remaining >
kMaxMessagePayloadBytes) {There was a problem hiding this comment.
Same as above: incoming_message_buffer_.size() <= kMaxMessagePayloadBytes is an invariant so the subtraction form avoids uint64_t addition overflow against untrusted frame_remaining.
a4776ed to
6fc5872
Compare
ebbb972 to
beef0b1
Compare
6fc5872 to
554f5c8
Compare
- Drop unmaintained external dependencies uWebSockets (v0.14) and libuv
(v1.40) from CMake, external rules, patches, and release NOTICES.
- Migrate Realtime Database WebSocket client (WebSocketClientImpl) to
libcurl's native WebSocket support (curl_ws_send, curl_ws_recv,
CURLOPT_CONNECT_ONLY = 2L).
- Give each WebSocket connection its own lightweight event thread and private
CURLM* handle using curl_multi_poll() and curl_multi_wakeup().
- Add stalled handshake deadline watchdog (5000 ms default).
- Centralize SDK transport security in app/rest/curl/tls_policy.{h,cc}
(TLS 1.2+ minimum, peer/hostname verification, and native OS certificate
trust store via Schannel on Windows, Keychain on macOS, and runtime CA bundle
detection on Linux).
- Add thread-safe reference-counted AcquireCurlGlobal() / ReleaseCurlGlobal()
and RAII handle deleters in app/rest/curl/curl_handles.{h,cc}.
- Initialize DatabaseInternal::persistence_enabled_ to false to prevent
reading uninitialized member variables on desktop platforms.
- Guard Logger::LogMessageImplV when parent_logger_ is null.
- Fix package.sh symbol hiding script generation for libfirebase_database.a.
- Add comprehensive WebSocket unit tests (16/16 passing) and concurrent
multi-app / multi-frame integration tests.
beef0b1 to
2a9856c
Compare
554f5c8 to
8fc4f1b
Compare
📚 Stacked PR Navigation
|
8fc4f1b to
878129e
Compare
94d75ff to
32dac60
Compare
Summary
Modernizes the desktop HTTP/REST transport (
app/rest/) by replacing the legacy 50 msselect()+curl_multi_fdset()polling loop with an event-driven process-wideCurlIoLoop(curl_multi_poll()+curl_multi_wakeup()), off-thread async completion delivery (CompletionQueue), thread-safe transfer control (TransferControl), and HTTP/2 multiplexing support.Architecture & Key Changes
1. Event-Driven HTTP Loop (
app/rest/curl/curl_io_loop.{h,cc})curl_multi_fdset()+select()+ 33–50 ms polling sleeps withcurl_multi_poll()+curl_multi_wakeup().FD_SETSIZE = 1024Cliff: Underselect(), any socket withfd >= 1024(common in Unreal/Unity desktop games with open asset packs) was invisible tofd_set, causing multi-second stalls when paired with a low-FD transfer or degrading into polling ticks.curl_multi_poll()uses OSpoll()/WSAPoll()with no descriptor ceiling.kIdlePollWaitMs(60 s) with 0% CPU wakeups; new transfers, pauses, resumes, and cancellations wake the loop immediately viacurl_multi_wakeup().2. Off-Thread Asynchronous Completion Delivery (
CompletionQueue)Response::MarkCompleted()andResponse::MarkFailed()callbacks on a dedicatedCompletionQueueworker thread rather than inline on theCurlIoLoopthread.Futurecontinuations, or synchronous token refresh RPCs from blocking or deadlocking the shared network I/O thread.CleanupTransportCurl()drains all queued completions before returning, preserving deterministic teardown semantics.3. Thread-Safe
ControllerCurl&TransferControlControllerCurlandTransportCurlwith a sharedstd::shared_ptr<TransferControl>state block.Controller::Pause()immediately afterPerform()(beforelibcurlhas opened a socket) latchespausedstate and returnsCURL_WRITEFUNC_PAUSE/CURL_READFUNC_PAUSEon the first data callback instead of being lost as a no-op.Controller::Cancel()wakes the loop immediately and aborts viaCURLOPT_XFERINFOFUNCTION, which also keepsBytesTransferred()andTransferSize()exact through 100% completion.4. HTTP/2 & RFC 9110 Response Parsing
CURL_HTTP_VERSION_2TLS(HTTP/2 over TLS with automatic HTTP/1.1 fallback) whenLibcurlSupportsHttp2()is true, and builds bundledlibcurlalongsidenghttp2.Response::ParseHeader()to parse HTTP/2 and HTTP/3 status lines (HTTP/2 200,HTTP/3 204), reasonless status lines (HTTP/1.1 200), and intermediate100 Continueresponses, with case-insensitive RFC 9110 header lookup.