Keep alive is slowing down shutdown

[cschreib-ibex]

Problem

We have an application running an HTTP server, which also ships web pages which regularly send GET requests to the server, to update the page content.

The application often needs to be "soft-restarted" (the application's process stays alive, but most of the classes are destroyed and re-created, including the HTTP server). When this happens, we are seeing long delays in shutting down the HTTP server, up to a few seconds.

I have traced it down to the "keep alive" logic, which possibly waits for the full duration configured in CPPHTTPLIB_KEEPALIVE_TIMEOUT_SECOND before giving up and letting the server shutdown. I believe the issue is essentially that the "keep alive" wait cannot be interrupted by the shutdown:

cpp-httplib/httplib.h

Lines 3256 to 3271 in 131bc6c

	process_server_socket_core(const std::atomic<socket_t> &svr_sock, socket_t sock,
	size_t keep_alive_max_count,
	time_t keep_alive_timeout_sec, T callback) {
	assert(keep_alive_max_count > 0);
	auto ret = false;
	auto count = keep_alive_max_count;
	while (svr_sock != INVALID_SOCKET && count > 0 &&
	select_read(sock, keep_alive_timeout_sec, 0) > 0) {
	auto close_connection = count == 1;
	auto connection_closed = false;
	ret = callback(close_connection, connection_closed);
	if (!ret || connection_closed) { break; }
	count--;
	}
	return ret;
	}

Here, select_read() will block for up to keep_alive_timeout_sec, even when the server socket is closed, because the condition svr_sock != INVALID_SOCKET is only checked before starting the call to select_read(), and not inside.

A possibly better implementation:

inline bool keep_alive(const std::atomic<socket_t> &svr_sock, socket_t sock,
                       time_t keep_alive_timeout_sec) {
  using namespace std::chrono;
  const auto start = steady_clock::now();
  while (svr_sock != INVALID_SOCKET &&
         (steady_clock::now() - start) < seconds(keep_alive_timeout_sec)) {
    if (select_read(sock, 0, 10000) > 0) {
      return true;
    }
  }

  return false;
}

template <typename T>
inline bool
process_server_socket_core(const std::atomic<socket_t> &svr_sock, socket_t sock,
                           size_t keep_alive_max_count,
                           time_t keep_alive_timeout_sec, T callback) {
  assert(keep_alive_max_count > 0);
  auto ret = false;
  auto count = keep_alive_max_count;
  while (keep_alive(svr_sock, sock, keep_alive_timeout_sec) && count > 0) {
    auto close_connection = count == 1;
    auto connection_closed = false;
    ret = callback(close_connection, connection_closed);
    if (!ret || connection_closed) { break; }
    count--;
  }
  return ret;
}

I see there used to be a keep_alive() function that looked similar (but still wasn't checking for svr_sock), and that was removed in 978a4f6. I lack the context to understand why that was done, but the implementation above passed all the tests.

How to reproduce

• Compile the following C++ code.
• Start the executable.

Output I get:

shutdown time: 5003 ms

C++ code:

#include "httplib.h"

#include <thread>
#include <chrono>

int main() {
    using namespace std::chrono;

    // Set up server listning in a thread.
    httplib::Server server;
    server.bind_to_port("0.0.0.0", 8080);

    server.set_mount_point("/", ".");

    // Wait until server is ready.
    auto thread = std::thread([&] { server.listen_after_bind(); });
    server.wait_until_ready();

    // Make a single client request, with keep alive enabled.
    httplib::Client client("127.0.0.1", 8080);
    client.set_keep_alive(true);
    client.Get("/index.html");

    // Stop.
    auto start = steady_clock::now();
    server.stop();
    thread.join();
    auto end = steady_clock::now();
    std::cout << "shutdown time: " << duration_cast<milliseconds>(end - start).count() << " ms" << std::endl;
}

[cschreib-ibex]

PR opened: #1960

[yhirose]

@cschreib-ibex thanks for the report. The reason why I removed keep_alive() function is that steady_clock::now() is too slow and caused a serious latency problem that my bench mark tool revealed. (There is the benchmark tool in /benchmark.)

I acknowledged the shutdown problem that you mentioned. I am not going to merge #1960, since it uses steady_clock::now(). But I'll try to find a solution for this.

[cschreib-ibex]

I'm surprised that *::now() would be a bottleneck; in the implementation I proposed, it is called every 10 milliseconds, and I doubt it takes nearly as long to execute. But I'm happy to investigate further, though I don't understand how to run the benchmark. I assume I need to run make monitor and make bench (maybe in the reverse order), but I don't know where the ali command comes from.

[yhirose]

@cschreib-ibex I resolve this issue at e0ebc43. The new implementation of keep_alive tries to avoid steady_clock::now() for the first time in the loop. So in most cases, this expensive function call won't take place.

Could you please test this latest version on your project? If it works, I'll bump the cpp-httplib version to 0.18.1. Thanks for your help!

[cschreib-ibex]

It works, thank you! PS: I'm still interesting in learning how to run the benchmarks, in case I need to propose another patch in the future.