Commit 98456bd8 authored by jan.koester's avatar jan.koester
Browse files

http: relay H1 Range responses synchronously instead of deferring to EPOLLOUT

RequestEvent's Range-GET branch sent headers-only (send(..., -1)) for
every protocol and relied on ResponseEvent being invoked again later
via EPOLLOUT re-arm to stream the body. For H1, reattachConnection()
only re-arms EPOLLOUT if something is still queued at that exact
moment -- a headers-only send leaves nothing queued, so that re-arm
never fires and the connection hangs forever having already promised
a real Content-Length.

Every <video> element requests with Range: by default, so this fires
on ordinary video playback. Each hang permanently parks one of
proxyplus's H1 dispatch worker threads; enough concurrent video
requests exhausts the whole (small, fixed) pool, at which point every
request through the proxy -- Range or not -- queues behind it forever,
wedging the entire site.

Fix: for H1 (whose dispatch already runs on its own dedicated
per-connection thread, not a shared epoll worker), relay the whole
body synchronously right here instead of deferring. H2/H3 keep the
deferred approach since _resumeH2Streams / the H3 stream pool
self-drive further ResponseEvent calls independent of epoll re-arming.

New test_http_range_streaming pins down the HttpClient streaming
contract this depends on. 8/8 tests passing.

Co-Authored-By: default avatarClaude Sonnet 5 <noreply@anthropic.com>
parent 2eeb452b
Loading
Loading
Loading
Loading
+77 −3
Changes for src/http/HttpProxyServer.cpp: 77 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -622,6 +622,14 @@ void HttpProxyController::RequestEvent(libhttppp::HttpRequest &curreq, const int
                // refers to the uncompressed representation), and neither app's media routes
                // send a Content-Encoding on these responses anyway.
                if (curreq.getHeaderData("range")) {
                    // H1 vs H2/H3 need genuinely different delivery mechanisms below, not
                    // just different plumbing -- see the two branches' own comments for why.
                    // getRequestVersion() is set from real parsed values for every protocol
                    // (parseH1 -> the literal wire version e.g. "HTTP/1.1"; parseH2/parseH3 ->
                    // the literal strings "HTTP/2"/"HTTP/3", libhttppp/src/http.cpp:4193,4272),
                    // so this is a reliable, already-public way to tell them apart here.
                    bool isH1 = curreq.getRequestVersion().rfind("HTTP/1", 0) == 0;

                    libhttppp::HttpResponse streamResp = client.GetStream(outreq);
                    int rangeStatus = streamResp.getStatusCode();

@@ -649,9 +657,75 @@ void HttpProxyController::RequestEvent(libhttppp::HttpRequest &curreq, const int

                    size_t rangeContentLength = streamResp.getContentLength();
                    curres.setContentLength(rangeContentLength);
                    // Headers only -- ResponseEvent below appends the body over however many
                    // further event-loop passes it takes (see HttpResponse::send's datalen=-1
                    // contract: skips both setContentLength and the body-append branch).

                    if (isH1) {
                        // proxyplus's shouldOffloadH1Dispatch (above) unconditionally offloads
                        // every H1 request onto libhttppp's H1 dispatch pool -- this call is
                        // already running on a thread dedicated to this one connection, not one
                        // of the few shared epoll workers, so blocking it for the whole transfer
                        // costs nothing else. That's not just an option here, it's a
                        // requirement: _dispatchH1Request's post-RequestEvent step
                        // (libhttppp/src/httpd.cpp) does one blocking flushSendData() and then
                        // hands the connection back to the event loop via reattachConnection(),
                        // which only arms EPOLLOUT if SendData or the socket's own
                        // hasPendingWrite() still has something queued at that exact moment
                        // (libnetplus/src/event/epoll.cpp's reattachConnection) -- with nothing
                        // left to flush after this headers-only send, no later EPOLLOUT (and so
                        // no later ResponseEvent call) would EVER fire, hanging the connection
                        // forever having already promised a real Content-Length. The deferred,
                        // ResponseEvent-driven approach the H2/H3 branch below uses relies on
                        // H2/H3's own self-driving resumption (_resumeH2Streams / H3's
                        // background pool loop, neither of which depend on epoll re-arming);
                        // H1 offload has no equivalent, so relay the whole body synchronously,
                        // right here, instead.
                        curres.send(curreq, "", -1);
                        if (!curreq.flushSendData()) {
                            // Client gave up reading -- nothing left to do; poolGuard below
                            // still closes rather than pools this connection since upstreamOk
                            // reflects the real response status, not delivery success.
                            upstreamOk = rangeStatus < 500;
                            lbGuard.ok = upstreamOk;
                            return;
                        }

                        char chunk[65536];
                        size_t remaining = rangeContentLength;
                        bool deliveryOk = true;
                        while (remaining > 0) {
                            size_t toRead = remaining < sizeof(chunk) ? remaining : sizeof(chunk);
                            size_t n;
                            try {
                                n = client.readBodyChunk(chunk, toRead); // blocking; 0 = body complete
                            } catch (const std::exception &e) {
                                std::cerr << "[http] streaming range response failed after "
                                          << (rangeContentLength - remaining) << "/"
                                          << rangeContentLength << " bytes: " << e.what() << std::endl;
                                deliveryOk = false;
                                break;
                            }
                            if (n == 0) break;
                            curreq.SendData.append(chunk, n);
                            if (!curreq.flushSendData()) { deliveryOk = false; break; }
                            remaining -= n;
                        }

                        upstreamOk = rangeStatus < 500;
                        lbGuard.ok = upstreamOk;
                        // Only hand the connection back for reuse if the whole body was
                        // actually drained -- an early break (client gave up, upstream cut
                        // off mid-body) leaves this HttpClient's read state unknown, exactly
                        // the same "don't pool a possibly-desynced connection" reasoning the
                        // H2/H3 branch below applies to a missing/zero Content-Length.
                        poolGuard.ok = upstreamOk && deliveryOk && remaining == 0;
                        return;
                    }

                    // H2/H3: mirrors blogi's own already-working GetStream()+ResponseEvent
                    // pattern for its H2-offloaded getimage route -- _resumeH2Streams (H2) and
                    // the H3 stream pool's background loop (both in libhttppp/src/httpd.cpp)
                    // self-drive further ResponseEvent calls without depending on epoll
                    // re-arming, so the deferred/extension-based approach the H1 branch above
                    // can't safely use is exactly right here.
                    curres.send(curreq, "", -1);

                    // Move the checked-out upstream connection into this request's own
+9 −0
Changes for test/CMakeLists.txt: 9 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -28,3 +28,12 @@ add_test(NAME test_http_proxy_paths COMMAND test_http_proxy_paths)
add_executable(test_upstreamconnectionpool test_upstreamconnectionpool.cpp)
target_link_libraries(test_upstreamconnectionpool PRIVATE proxyplus_core)
add_test(NAME test_upstreamconnectionpool COMMAND test_upstreamconnectionpool)

# Focused test for the libhttppp::HttpClient streaming API (GetStream() +
# readBodyChunkNonBlocking()) that HttpProxyController::RequestEvent's Range-GET branch
# depends on (see src/http/HttpProxyServer.cpp). Links proxyplus_core purely for convenience
# (it already pulls in libhttppp/libnetplus with the right include/link setup); it does not
# touch any proxyplus-specific class.
add_executable(test_http_range_streaming test_http_range_streaming.cpp)
target_link_libraries(test_http_range_streaming PRIVATE proxyplus_core)
add_test(NAME test_http_range_streaming COMMAND test_http_range_streaming)
+240 −0
Changes for test/test_http_range_streaming.cpp: 240 added lines, 0 removed lines.
Original line number Diff line number Diff line
// Focused regression test for the libhttppp::HttpClient streaming API that
// HttpProxyController::RequestEvent's new Range-GET branch (src/http/HttpProxyServer.cpp)
// depends on: GetStream() + readBodyChunkNonBlocking(). This deliberately does NOT stand up a
// full HttpProxyController (that needs a ConfigStore/AuthService/PluginManager/LoadBalancer/
// UpstreamConnectionPool just to construct one -- see docs/ARCHITECTURE.md and the other test_*
// files here, all of which are unit-level). Instead it drives the exact same HttpClient calls
// proxyplus's ResponseEvent loop makes, against a trivial fake upstream, to pin down the
// contract proxyplus's new code relies on:
//
//   1. GetStream() returns the parsed response headers synchronously and does NOT populate
//      HttpClient::lastResponse()/lastStatusCode() (proxyplus reads status/headers/content-
//      length off the HttpResponse GetStream() itself returns, never off lastResponse() --
//      this test fails if that assumption ever stops holding).
//   2. readBodyChunkNonBlocking() returns 0 (not -1) while more bytes are expected but not yet
//      available -- this is what drives ResponseEvent's ResponsePending/retry signal.
//   3. readBodyChunkNonBlocking() reproduces the exact bytes sent, across however many partial
//      reads it takes.
//   4. Once the declared Content-Length has been fully delivered, a further
//      readBodyChunkNonBlocking() call returns (size_t)-1 -- proxyplus's ResponseEvent only
//      relies on its own produced/send_size bookkeeping to stop, but never actually calls
//      readBodyChunkNonBlocking() again past that point in the happy path, so this pins down
//      that the -1 sentinel really does mean "done", not "broken", for a clean Content-Length
//      body (see HttpProxyServer.cpp's ResponseEvent, which treats -1 as an upstream failure --
//      safe only because the happy path never reaches it after produced>=send_size).

#include <cstring>
#include <iostream>
#include <thread>
#include <vector>

#include <netplus/socket.h>

#include <httppp/http.h>

namespace {

// This project's CMAKE_BUILD_TYPE=RelWithDebInfo passes -DNDEBUG to every target, including
// every test/*.cpp file here -- which compiles bare assert() out entirely (verified directly:
// `assert(1==2)` under this exact build's flags runs straight through, no abort). A check that
// matters for this test's actual pass/fail verdict must not be a bare assert() for that reason;
// this small helper always evaluates its condition and fails loudly (message + process exit)
// regardless of NDEBUG.
int g_failures = 0;
void check(bool cond, const char *what) {
    if (!cond) {
        std::cerr << "FAIL: " << what << std::endl;
        ++g_failures;
    }
}

} // namespace

namespace {

// One-shot fake HTTP/1.1 upstream: accepts a single connection, discards whatever request it
// sent (this test doesn't care what was asked for), then writes a 206 Partial Content response
// with a known Content-Length -- split across two writes with a short delay between them so the
// client-side non-blocking reader is forced to observe a genuine "no data yet" gap, not just a
// single read that happens to get everything at once.
class FakeRangeUpstream {
public:
    FakeRangeUpstream(int port, std::vector<char> body)
        : _portNum(port), _body(std::move(body)), _sock("127.0.0.1", port, 8, -1) {
        _sock.bind();
        _sock.listen();
        _thread = std::thread([this] { run(); });
    }
    ~FakeRangeUpstream() {
        if (_thread.joinable()) _thread.join();
    }

private:
    void run() {
        std::unique_ptr<netplus::socket> csock = std::make_unique<netplus::tcp>(-1);
        try {
            _sock.accept(csock, false);
        } catch (...) {
            return;
        }
        if (!csock) return;

        // Drain whatever the client sent (headers end at "\r\n\r\n") -- best-effort, this test
        // doesn't inspect the request.
        {
            char buf[4096];
            netplus::buffer rb(buf, sizeof(buf));
            std::string recvd;
            for (int i = 0; i < 50 && recvd.find("\r\n\r\n") == std::string::npos; ++i) {
                try {
                    rb.size = sizeof(buf);
                    size_t n = csock->recvData(rb, 0);
                    if (n > 0) recvd.append(buf, n);
                    else break;
                } catch (...) {
                    std::this_thread::sleep_for(std::chrono::milliseconds(5));
                }
            }
        }

        std::string headers = "HTTP/1.1 206 Partial Content\r\n"
                               "Content-Type: video/mp4\r\n"
                               "Content-Range: bytes 0-" + std::to_string(_body.size() - 1) +
                               "/" + std::to_string(_body.size()) + "\r\n"
                               "Content-Length: " + std::to_string(_body.size()) + "\r\n"
                               "\r\n";

        sendAll(*csock, headers.data(), headers.size());

        // First half now, second half after a short delay -- gives the client a real window
        // where readBodyChunkNonBlocking() must return 0 (not -1, not a wrong byte count)
        // because the rest genuinely hasn't arrived yet.
        size_t half = _body.size() / 2;
        sendAll(*csock, _body.data(), half);
        std::this_thread::sleep_for(std::chrono::milliseconds(200));
        sendAll(*csock, _body.data() + half, _body.size() - half);

        // Keep the socket around a little longer so the client's final reads land on a live
        // connection rather than a reset one -- this test only cares about correct streaming,
        // not connection-close/pooling behavior (that's UpstreamConnectionPool's own test).
        std::this_thread::sleep_for(std::chrono::milliseconds(200));
    }

    static void sendAll(netplus::socket &sock, const char *data, size_t len) {
        size_t off = 0;
        while (off < len) {
            netplus::buffer buf(const_cast<char *>(data + off), len - off);
            size_t n;
            try {
                n = sock.sendData(buf, 0);
            } catch (...) {
                std::this_thread::sleep_for(std::chrono::milliseconds(5));
                continue;
            }
            if (n == 0) {
                std::this_thread::sleep_for(std::chrono::milliseconds(5));
                continue;
            }
            off += n;
        }
    }

    int _portNum;
    std::vector<char> _body;
    netplus::tcp _sock;
    std::thread _thread;
};

std::vector<char> makeBody(size_t n) {
    std::vector<char> body(n);
    for (size_t i = 0; i < n; ++i) body[i] = static_cast<char>(i % 256);
    return body;
}

} // namespace

int main() {
    const int port = 19901;
    const size_t kBodySize = 200000; // > any single readBodyChunkNonBlocking buffer used below
    std::vector<char> expected = makeBody(kBodySize);

    FakeRangeUpstream server(port, expected);

    libhttppp::HttpUrl url("http://127.0.0.1:" + std::to_string(port) + "/video");
    // vers=0: plain HTTP/1.1, matching test_upstreamconnectionpool.cpp's convention for a
    // cleartext fake upstream (no HTTP/2 or HTTP/3 negotiation to race against this test's
    // single-shot accept()).
    libhttppp::HttpClient client(url, /*vers=*/0, /*timeoutSec=*/5);

    libhttppp::HttpRequest req;
    req.setRequestURL("/video");
    req.setHeaderData("range")->push_back("bytes=0-");

    libhttppp::HttpResponse resp = client.GetStream(req);

    // --- Headers arrived synchronously and carry the real status/length ---
    check(resp.getStatusCode() == 206, "GetStream() status code == 206");
    check(resp.getContentLength() == kBodySize, "GetStream() Content-Length == kBodySize");
    std::cout << "PASS: GetStream() returned 206 with Content-Length=" << kBodySize << std::endl;

    // --- GetStream() must NOT populate lastResponse()/lastStatusCode() -- proxyplus's new
    // Range-GET branch relies on this and reads status/headers/content-length off the
    // HttpResponse GetStream() itself returned instead. ---
    check(client.lastResponse() == nullptr, "lastResponse() untouched by GetStream()");
    check(client.lastStatusCode() == 0, "lastStatusCode() untouched by GetStream()");
    std::cout << "PASS: GetStream() left lastResponse()/lastStatusCode() untouched" << std::endl;

    // --- Drain the body exactly the way HttpProxyController::ResponseEvent does: bounded
    // non-blocking reads, treating 0 as "not yet, retry" and accumulating into a buffer. ---
    std::vector<char> collected;
    collected.reserve(kBodySize);
    bool sawWouldBlock = false;
    int idleSpins = 0;
    while (collected.size() < kBodySize) {
        char chunk[65536];
        size_t n = client.readBodyChunkNonBlocking(chunk, sizeof(chunk));
        if (n == static_cast<size_t>(-1)) {
            std::cerr << "FAIL: readBodyChunkNonBlocking() returned -1 after only "
                      << collected.size() << "/" << kBodySize << " bytes" << std::endl;
            return 1;
        }
        if (n > 0) {
            collected.insert(collected.end(), chunk, chunk + n);
        } else {
            sawWouldBlock = true;
            if (++idleSpins > 2000) { // ~10s worst case at 5ms/spin -- generous vs the fake
                                       // upstream's 200ms artificial gap
                std::cerr << "FAIL: readBodyChunkNonBlocking() never produced the remaining "
                          << (kBodySize - collected.size()) << " bytes" << std::endl;
                return 1;
            }
            std::this_thread::sleep_for(std::chrono::milliseconds(5));
        }
    }

    check(sawWouldBlock, "at least one real would-block (0) was observed during the drain");
    check(collected.size() == kBodySize, "collected byte count == kBodySize");
    check(std::memcmp(collected.data(), expected.data(), kBodySize) == 0,
          "collected bytes match the fake upstream's body exactly");
    std::cout << "PASS: readBodyChunkNonBlocking() reproduced all " << kBodySize
              << " bytes exactly, including across a real would-block gap" << std::endl;

    // --- Past the declared Content-Length, the stream reports itself done via -1, not a
    // wrong byte count or an exception -- this is what makes proxyplus's own
    // produced>=send_size bookkeeping (which stops calling before this point) a safe
    // simplification rather than a mismatch with what the client object itself believes. ---
    char extra[16];
    size_t trailing = client.readBodyChunkNonBlocking(extra, sizeof(extra));
    check(trailing == static_cast<size_t>(-1),
          "readBodyChunkNonBlocking() returns -1 once Content-Length bytes are delivered");
    std::cout << "PASS: readBodyChunkNonBlocking() reports completion via -1 once "
                 "Content-Length bytes have been delivered"
              << std::endl;

    if (g_failures > 0) {
        std::cerr << g_failures << " check(s) failed." << std::endl;
        return 1;
    }
    std::cout << "All HttpClient Range-streaming tests passed." << std::endl;
    return 0;
}