From a78e83e17f575c2f2641fd03edec1b9d49714fe4 Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 20:20:25 +0200 Subject: [PATCH 1/6] Add "Known Issues" paragraph --- README.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index e47b3e3..75c7e28 100644 --- a/README.md +++ b/README.md @@ -10,4 +10,10 @@ This is a minimal web server designed to serve my blog. I'm writing it to be rob - Single core (This will probably change when I get a better VPS) # How I test it -I'm still testing manually. I usually stress test the server locally using `wrk` and see if it breaks. I also test it under `valgrind` and sanitizers. \ No newline at end of file +I'm still testing manually. I usually stress test the server locally using `wrk` and see if it breaks. I also test it under `valgrind` and sanitizers. + +# Known issues: +- When idle connections timeout, the server sends 411 +- Server tries to keep connections alive even when peer sends `Connection: Close` +- Server replies to HTTP/1.0 clients as HTTP/1.1 +- Since poll is edge triggered, when the server is full and can't accept all new connections the remaining ones are left waiting until some other event wakes up poll() \ No newline at end of file From 0e17e447e7077c54866be38d262e1c52578776ab Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 22:49:22 +0200 Subject: [PATCH 2/6] Don't ignore Connection headers from clients --- README.md | 6 +++--- serve.c | 48 +++++++++++++++++++++++++++++++++++++++++------- tests/test.py | 46 ++++++++++++++++++++++++++++++++++++---------- 3 files changed, 80 insertions(+), 20 deletions(-) diff --git a/README.md b/README.md index 75c7e28..ab471ff 100644 --- a/README.md +++ b/README.md @@ -12,8 +12,8 @@ This is a minimal web server designed to serve my blog. I'm writing it to be rob # How I test it I'm still testing manually. I usually stress test the server locally using `wrk` and see if it breaks. I also test it under `valgrind` and sanitizers. -# Known issues: -- When idle connections timeout, the server sends 411 +# Known Issues +- When idle connections timeout, the server sends 408 - Server tries to keep connections alive even when peer sends `Connection: Close` - Server replies to HTTP/1.0 clients as HTTP/1.1 -- Since poll is edge triggered, when the server is full and can't accept all new connections the remaining ones are left waiting until some other event wakes up poll() \ No newline at end of file +- Since poll is edge triggered, when the server is full and can't accept all new connections the remaining ones are left waiting until some other event wakes up poll() diff --git a/serve.c b/serve.c index 5798284..e47f07b 100644 --- a/serve.c +++ b/serve.c @@ -18,8 +18,8 @@ #include #define PORT 8080 -#define SHOW_IO 0 -#define SHOW_REQUESTS 1 +#define SHOW_IO 1 +#define SHOW_REQUESTS 0 #define REQUEST_TIMEOUT_SEC 5 #define CLOSING_TIMEOUT_SEC 2 #define CONNECTION_TIMEOUT_SEC 60 @@ -353,11 +353,13 @@ string trim(string s) size_t cur = 0; while (cur < s.size && is_space(s.data[cur])) cur++; - + if (cur == s.size) { s.data = ""; s.size = 0; } else { + s.data += cur; + s.size -= cur; while (is_space(s.data[s.size-1])) s.size--; } @@ -711,6 +713,7 @@ typedef struct { ByteQueue output; int served_count; bool closing; + bool keep_alive; uint64_t creation_time; uint64_t start_time; } Connection; @@ -927,17 +930,29 @@ int num_conns = 0; bool should_keep_alive(Connection *conn) { + // Don't keep alive if the peer doesn't want to + if (conn->keep_alive == false) { + DEBUG("Not keeping alive because peer wants to close\n"); + return false; + } + // Don't keep alive if the request is too old - if (now - conn->creation_time > CONNECTION_TIMEOUT_SEC * 1000) + if (now - conn->creation_time > CONNECTION_TIMEOUT_SEC * 1000) { + DEBUG("Not keeping alive because the connection is too old\n"); return false; + } // Don't keep alive if we served a lot of requests to this connection - if (conn->served_count > 100) + if (conn->served_count > 100) { + DEBUG("Not keeping alive because too many requests were served using this connection\n"); return false; + } // Don't keep alive if the server is more than 70% full - if (num_conns > 0.7 * MAX_CONNECTIONS) + if (num_conns > 0.7 * MAX_CONNECTIONS) { + DEBUG("Not keeping alive because the server is more than 70%% full\n"); return false; + } return true; } @@ -1053,7 +1068,21 @@ bool respond_to_available_requests(struct pollfd *polldata, Connection *conn) break; // Request wasn't completely received yet // Reset the request timer - conns->start_time = now; + conn->start_time = now; + + conn->keep_alive = false; + string keep_alive_header; + if (find_header(&request, LIT("Connection"), &keep_alive_header)) { + DEBUG("Found Connection header\n"); + if (string_match_case_insensitive(trim(keep_alive_header), LIT("Keep-Alive"))) { + conn->keep_alive = true; + DEBUG("Matched Keep-Alive (%.*s)\n", (int) trim(keep_alive_header).size, trim(keep_alive_header).data); + } else { + DEBUG("Didn't match Keep-Alive (%.*s)\n", (int) trim(keep_alive_header).size, trim(keep_alive_header).data); + } + } else { + DEBUG("No Connection header\n"); + } // Respond ResponseBuilder builder; @@ -1069,6 +1098,11 @@ bool respond_to_available_requests(struct pollfd *polldata, Connection *conn) polldata->events |= POLLOUT; polldata->revents |= POLLOUT; } + if (!conn->keep_alive) { + polldata->events &= ~POLLIN; + conn->closing = true; + conn->start_time = now; + } pipeline_count++; if (pipeline_count == 10) { diff --git a/tests/test.py b/tests/test.py index 35ced7f..9b36d73 100644 --- a/tests/test.py +++ b/tests/test.py @@ -6,25 +6,24 @@ @dataclass class Delay: - ms: int + ms: float @dataclass class Send: data: bytes - timeout: Optional[int] = None + timeout: Optional[float] = None @dataclass class Recv: data: bytes - timeout: Optional[int] = None + timeout: Optional[float] = None @dataclass class Close: pass def print_bytes(prefix, data): - lines = str(data).split("\r\n") - print(prefix, f"\\r\\n\n{prefix}".join(lines), sep="") + print(prefix, f"\\r\\n\n{prefix}".join(data.decode("utf-8").split("\r\n")), sep="") def run_test(test, addr, port): @@ -44,7 +43,7 @@ def run_test(test, addr, port): n = conn.send(data[sent:]) if n == 0: break - #print_bytes("< ", data[sent:sent+n]) + print_bytes("< ", data[sent:sent+n]) sent += n case Recv(data, timeout): @@ -54,7 +53,7 @@ def run_test(test, addr, port): chunk = conn.recv(len(data) - count) if chunk == b"": break - #print_bytes("> ", chunk) + print_bytes("> ", chunk) chunks.append(chunk) count += len(chunk) received = b''.join(chunks) @@ -69,31 +68,58 @@ def run_test(test, addr, port): case _: pass + tests = [ [ + # Test "Connection: Close" Send(b"GET / HTTP/1.1\r\nConnection: Close\r\n\r\n"), - Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Close\r\n\r\n"), + Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Close\r\nContent-Length: 15 \r\n\r\nNothing here :|"), Close(), ], [ + # Test "Connection: Keep-Alive" Send(b"GET / HTTP/1.1\r\nConnection: Keep-Alive\r\n\r\n"), Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Keep-Alive\r\nContent-Length: 15 \r\n\r\nNothing here :|"), + Send(b"GET / HTTP/1.1\r\nConnection: Close\r\n\r\n"), + Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Close\r\nContent-Length: 15 \r\n\r\nNothing here :|"), + Close(), ], [ Send(b"XXX\r\n\r\n"), Recv(b"HTTP/1.1 400 Bad Request\r\nConnection: Close\r\n\r\n"), ], + [ + Send(b"GET /hello HTTP/1.1\r\nConnection: Keep-Alive\r\n\r\n"), + Recv(b"HTTP/1.1 200 OK\r\nConnection: Keep-Alive\r\nContent-Length: 13 \r\n\r\nHello, world!"), + ], + [ + # Test request timeout + Send(b"GET /hel"), + Delay(6), + Recv(b"HTTP/1.1 408 Request Timeout\r\nConnection: Close\r\n\r\n"), + Close() + ], + [ + # Test idle connection timeout + Delay(6), + Close() + ], ] p = subprocess.Popen(['../serve_cov'], stdout=subprocess.PIPE, stderr=subprocess.PIPE) time.sleep(0.5) +total = 0 +passed = 0 for i, test in enumerate(tests): try: run_test(test, "127.0.0.1", 8080) + print("Test", i, "passed\n") + passed += 1 except Exception as e: - print("Test", i, "failed:", e.with_traceback(None)) - + print("Test", i, "failed:", e.with_traceback(None), "\n") + total += 1 +print("passed: ", passed, "/", total, sep="") p.terminate() p.wait() From c90cead09ce5f3605459c5d08ebf8a16bad33c33 Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 22:54:55 +0200 Subject: [PATCH 3/6] Don't send 408 to idle connections --- serve.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/serve.c b/serve.c index e47f07b..85f88dc 100644 --- a/serve.c +++ b/serve.c @@ -1311,16 +1311,22 @@ int main(int argc, char **argv) log_data(LIT("Closing timeout\n")); } else { // Request timeout - byte_queue_write(&conn->output, LIT( - "HTTP/1.1 408 Request Timeout\r\n" - "Connection: Close\r\n" - "\r\n")); - conn->closing = true; - conn->start_time = now; - pollarray[i].events &= ~POLLIN; - pollarray[i].events |= POLLOUT; - pollarray[i].revents |= POLLOUT; - log_data(LIT("Request timeout\n")); + if (byte_queue_size(&conn->input) == 0) { + // Connection was idle, so just close it + remove = true; + log_data(LIT("Idle connection timeout\n")); + } else { + byte_queue_write(&conn->output, LIT( + "HTTP/1.1 408 Request Timeout\r\n" + "Connection: Close\r\n" + "\r\n")); + conn->closing = true; + conn->start_time = now; + pollarray[i].events &= ~POLLIN; + pollarray[i].events |= POLLOUT; + pollarray[i].revents |= POLLOUT; + log_data(LIT("Request timeout\n")); + } } } else if (!remove && (pollarray[i].revents & POLLIN)) { From fd9b2d8812a90851cc2b73f122977d2bc29d00fa Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 22:55:50 +0200 Subject: [PATCH 4/6] Update todo list --- README.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/README.md b/README.md index ab471ff..63b583c 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,5 @@ This is a minimal web server designed to serve my blog. I'm writing it to be rob I'm still testing manually. I usually stress test the server locally using `wrk` and see if it breaks. I also test it under `valgrind` and sanitizers. # Known Issues -- When idle connections timeout, the server sends 408 -- Server tries to keep connections alive even when peer sends `Connection: Close` - Server replies to HTTP/1.0 clients as HTTP/1.1 - Since poll is edge triggered, when the server is full and can't accept all new connections the remaining ones are left waiting until some other event wakes up poll() From 6f9b4794f5db77d6945f1cdcf88995508c44f8fc Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 23:00:35 +0200 Subject: [PATCH 5/6] Change configs for release builds --- Makefile | 2 +- serve.c | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Makefile b/Makefile index 620b147..b1cdea1 100644 --- a/Makefile +++ b/Makefile @@ -7,7 +7,7 @@ serve_cov: serve.c gcc serve.c -o serve_cov -Wall -Wextra -O2 -fprofile-arcs -ftest-coverage -lgcov serve: serve.c - gcc serve.c -o serve -Wall -Wextra -O2 -DNDEBUG + gcc serve.c -o serve -Wall -Wextra -O2 -DNDEBUG -DRELEASE report: lcov --capture --directory . --output-file coverage.info diff --git a/serve.c b/serve.c index 85f88dc..19788bf 100644 --- a/serve.c +++ b/serve.c @@ -17,16 +17,22 @@ #include #include +#ifdef RELEASE +#define PORT 80 +#define LOG_DIRECTORY_SIZE_LIMIT_MB (25 * 1024) +#else #define PORT 8080 -#define SHOW_IO 1 -#define SHOW_REQUESTS 0 +#define LOG_DIRECTORY_SIZE_LIMIT_MB 10 +#endif + +#define SHOW_IO 0 +#define SHOW_REQUESTS 1 #define REQUEST_TIMEOUT_SEC 5 #define CLOSING_TIMEOUT_SEC 2 #define CONNECTION_TIMEOUT_SEC 60 #define LOG_BUFFER_SIZE (1<<20) #define LOG_BUFFER_LIMIT (1<<24) #define LOG_FLUSH_TIMEOUT_SEC 3 -#define LOG_DIRECTORY_SIZE_LIMIT_MB 10 #define INPUT_BUFFER_LIMIT_MB 1 #ifndef NDEBUG @@ -1188,6 +1194,9 @@ bool write_to_socket(int fd, ByteQueue *queue) int main(int argc, char **argv) { + (void) argc; + (void) argv; + signal(SIGTERM, handle_sigterm); signal(SIGQUIT, handle_sigterm); signal(SIGINT, handle_sigterm); From 5220bf0ebb4361a88bc52de6da26929021f65c9d Mon Sep 17 00:00:00 2001 From: Francesco Cozzuto Date: Fri, 13 Sep 2024 23:12:48 +0200 Subject: [PATCH 6/6] Fix case insensitive string compare --- serve.c | 2 +- tests/test.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/serve.c b/serve.c index 19788bf..18a14ff 100644 --- a/serve.c +++ b/serve.c @@ -333,7 +333,7 @@ int parse_request_head(string str, Request *request) char to_lower(char c) { - if (c >= 'A' || c <= 'Z') + if (c >= 'A' && c <= 'Z') return c - 'A' + 'a'; else return c; diff --git a/tests/test.py b/tests/test.py index 9b36d73..f28ecbe 100644 --- a/tests/test.py +++ b/tests/test.py @@ -84,6 +84,14 @@ def run_test(test, addr, port): Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Close\r\nContent-Length: 15 \r\n\r\nNothing here :|"), Close(), ], + [ + # Test that the connection header is insensitive to case and whitespace + Send(b"GET / HTTP/1.1\r\nConnection: keEp-ALiVE \r\n\r\n"), + Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Keep-Alive\r\nContent-Length: 15 \r\n\r\nNothing here :|"), + Send(b"GET / HTTP/1.1\r\nConnection: closE\r\n\r\n"), + Recv(b"HTTP/1.1 404 Not Found\r\nConnection: Close\r\nContent-Length: 15 \r\n\r\nNothing here :|"), + Close(), + ], [ Send(b"XXX\r\n\r\n"), Recv(b"HTTP/1.1 400 Bad Request\r\nConnection: Close\r\n\r\n"),