From 667edacaa3805355f687f535633dbac6f84d7ed2 Mon Sep 17 00:00:00 2001
From: Tom Dryer <tomdryer.com@gmail.com>
Date: Sun, 13 Jun 2021 18:44:55 -0700
Subject: [PATCH] Fix hung connection from consecutive requests (#7)

A client making quick consecutive requests with keep-alive, such as `ab`
with `-k`, can cause the connection to become hung.

This happens because of an optimization in `http_poll` function. When a
connection state becomes `DONE`, `httpd_poll` recycles the connection
and immediately calls `poll_recv_request`. However, it doesn't handle
this resulting in the connection state becoming `DONE` again. If this
occurs, the state stays in `DONE`, and the further calls to `httpd_poll`
ignore the connection.

Fix this by calling `poll_recv_request` in a loop until the state is no
longer `DONE`.

* Enable TCP_NODELAY optimization

It looks like `TCP_NODELAY` was disabled due to the bug fixed in the
previous commit. Enabling it substantially improves keep-alive
performance with `ab`:

Before:

```
Time per request:       0.272 [ms] (mean)
```

After:

```
Time per request:       0.033 [ms] (mean)
```

* Remove keep-alive optimization from `httpd_poll`

Benchmarking with `ab` shows that bypassing `select` for keep-alive
connections in the `DONE` state doesn't significantly impact
performance. Since this optimization previously caused a bug, remove it.
---
 TODO        | 2 --
 darkhttpd.c | 8 +-------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/TODO b/TODO
index d94db58..0155df9 100644
--- a/TODO
+++ b/TODO
@@ -1,3 +1 @@
-- keepalive performance against ab sucks
-- keepalive+TCP_NODELAY hangs!
 - send headers using sendfile syscall - fewer packets
diff --git a/darkhttpd.c b/darkhttpd.c
index 268628a..021faab 100644
--- a/darkhttpd.c
+++ b/darkhttpd.c
@@ -817,13 +817,11 @@ static void init_sockin(void) {
                    &sockopt, sizeof(sockopt)) == -1)
         err(1, "setsockopt(SO_REUSEADDR)");
 
-#if 0
     /* disable Nagle since we buffer everything ourselves */
     sockopt = 1;
     if (setsockopt(sockin, IPPROTO_TCP, TCP_NODELAY,
             &sockopt, sizeof(sockopt)) == -1)
         err(1, "setsockopt(TCP_NODELAY)");
-#endif
 
 #ifdef TORTURE
     /* torture: cripple the kernel-side send buffer so we can only squeeze out
@@ -2495,7 +2493,7 @@ static void httpd_poll(void) {
     LIST_FOREACH_SAFE(conn, &connlist, entries, next) {
         switch (conn->state) {
         case DONE:
-            /* do nothing */
+            /* do nothing, no connection should be left in this state */
             break;
 
         case RECV_REQUEST:
@@ -2586,10 +2584,6 @@ static void httpd_poll(void) {
                 free(conn);
             } else {
                 recycle_connection(conn);
-                /* and go right back to recv_request without going through
-                 * select() again.
-                 */
-                poll_recv_request(conn);
             }
         }
     }