cpp-httplib icon indicating copy to clipboard operation
cpp-httplib copied to clipboard

Add {Response,DataSink}::is_alive

Open ochafik opened this issue 1 year ago • 2 comments

This is to be able to detect if the client connection was lost.

Context: https://github.com/yhirose/cpp-httplib/issues/1952 / https://github.com/ggerganov/llama.cpp/pull/9679

As suggested by @ngxson, since is_writable / select_write may be a bit heavyweight, I tried using just select_read (given it's in a context when every other thread is done reading) but it didn't do the trick alone, so settled for is_socket_alive (which uses select_read + read_socket): seems to work.

Update: I finally realized having is_alive on DataSink isn't enough, so moved it to Response itself (by lack of a better place?). In the llama.cpp use case we need to test for liveness before status / headers are even written back. Added a tiny test to exercise this.

Tested (no new failures): cmake -B build -DHTTPLIB_TEST=1 -DHTTPLIB_REQUIRE_OPENSSL=0 -DHTTPLIB_COMPILE=1 -DHTTPLIB_TEST=1 -DHTTPLIB_REQUIRE_ZLIB=1 -DHTTPLIB_REQUIRE_BROTLI=1 && cmake --build build -j && ctest --test-dir build -j

ochafik avatar Oct 04 '24 04:10 ochafik

@yhirose What do you think about this approach?

ochafik avatar Oct 10 '24 20:10 ochafik

@ochafik thanks for the pull request. I took a look at the change, but I am not comfortable to expose such a method in Response.

Also your test code looks working, but it's a very very dangerous code actually.

If I make the following change in Server::process_request in httplib.h to make a response object on heap instead of stack as below, the hidden problem will be revealed.

  // Response res; // Original code on stack
  auto resp = detail::make_unique<Response>(); // on heap
  Response& res = *resp;

We get a crash when calling res.is_alive() the second time in the loop, since res has already been gone.

        while (count > 0 && res.is_alive()) {

I added debug print statements as below, and tested.

        std::cout << "enter loop" << std::endl;
        while (count > 0 && res.is_alive()) {
          this_thread::sleep_for(chrono::milliseconds(10));
        }
        std::cout << "leave loop" << std::endl;

Here is the result.

~/Projects/cpp-httplib/test$ make test && ./test --gtest_filter="*ClientCloseDetectionOnResponse*"
clang++ -o test -I.. -g -std=c++11 -I. -Wall -Wextra -Wtype-limits -Wconversion -Wshadow  test.cc include_httplib.cc gtest/gtest-all.cc gtest/gtest_main.cc -DCPPHTTPLIB_OPENSSL_SUPPORT -I/opt/homebrew/opt/openssl@3/include -L/opt/homebrew/opt/openssl@3/lib -lssl -lcrypto -DCPPHTTPLIB_USE_CERTS_FROM_MACOSX_KEYCHAIN -framework CoreFoundation -framework Security -DCPPHTTPLIB_ZLIB_SUPPORT -lz -DCPPHTTPLIB_BROTLI_SUPPORT -I/opt/homebrew/opt/brotli/include -L/opt/homebrew/opt/brotli/lib -lbrotlicommon -lbrotlienc -lbrotlidec -pthread -lcurl
Running main() from gtest/gtest_main.cc
Note: Google Test filter = *ClientCloseDetectionOnResponse*
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from LongPollingTest
[ RUN      ] LongPollingTest.ClientCloseDetectionOnResponse
enter loop
libc++abi: terminating due to uncaught exception of type std::__1::bad_function_call: std::exception
Abort trap: 6

So at this point, this pull request is not acceptable...

yhirose avatar Oct 10 '24 23:10 yhirose

I'll close it for now.

yhirose avatar Oct 20 '24 12:10 yhirose