From 3a5aa87853335f73c29ad407b33c8e1c285a77ee Mon Sep 17 00:00:00 2001
From: Karl Seguin <k@openmymind.io>
Date: Thu, 5 Jun 2025 12:32:09 +0800
Subject: [PATCH] Optimize the lifecycle of async requests

Async HTTP request work by emitting a "Progress" object to a callback. This
object has a "done" flag which, when `true`, indicates that all data has been
emitting and no future "Progress" objects will be sent.

Callers like XHR buffer the response and wait for "done = true" to then process
the request.

The HTTP client relies on two important object pools: the connection and the
state (with all the buffers for reading/writing).

In its current implementation, the async flow does not release these pooled
objects until the final callback has returned. At best, this is inefficient:
we're keeping the connection and state objects checked out for longer than they
have to be. At worse, it can lead to a deadlock. If the calling code issues a
new request when done == true, we'll eventually run out of state objects in the
pool.

This commit now releases the state objects before emit the final "done" Progress
message. For this to work, this final message will always have null data and
an empty header object.
---
 src/browser/xhr/xhr.zig |  3 +--
 src/http/client.zig     | 40 ++++++++++++++++++++++++++++++++++------
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/src/browser/xhr/xhr.zig b/src/browser/xhr/xhr.zig
index d65f9db8..3f1dfc2a 100644
--- a/src/browser/xhr/xhr.zig
+++ b/src/browser/xhr/xhr.zig
@@ -468,7 +468,6 @@ pub const XMLHttpRequest = struct {
 
         if (progress.first) {
             const header = progress.header;
-
             log.debug(.http, "request header", .{
                 .source = "xhr",
                 .url = self.url,
@@ -522,7 +521,7 @@ pub const XMLHttpRequest = struct {
         log.info(.http, "request complete", .{
             .source = "xhr",
             .url = self.url,
-            .status = progress.header.status,
+            .status = self.response_status,
         });
 
         // Not that the request is done, the http/client will free the request
diff --git a/src/http/client.zig b/src/http/client.zig
index 81b11cd7..968402b7 100644
--- a/src/http/client.zig
+++ b/src/http/client.zig
@@ -828,6 +828,7 @@ fn AsyncHandler(comptime H: type, comptime L: type) type {
             wait,
             done,
             need_more,
+            handler_error,
         };
 
         fn deinit(self: *Self) void {
@@ -966,12 +967,35 @@ fn AsyncHandler(comptime H: type, comptime L: type) type {
             switch (status) {
                 .wait => {},
                 .need_more => self.receive(),
+                .handler_error => {
+                    // handler should never have been called if we're redirecting
+                    std.debug.assert(self.redirect == null);
+                    self.request.requestCompleted(self.reader.response);
+                    self.deinit();
+                    return;
+                },
                 .done => {
                     const redirect = self.redirect orelse {
+                        var handler = self.handler;
                         self.request.requestCompleted(self.reader.response);
                         self.deinit();
+
+                        // Emit the done chunk. We expect the caller to do
+                        // processing once the full request is completed. By
+                        // emiting this AFTER we've relreased the connection,
+                        // we free the connection and its state for re-use.
+                        // If we don't do this this way, we can end up with
+                        // _a lot_ of pending request/states.
+                        // DO NOT USE `self` here, it's no longer valid.
+                        handler.onHttpResponse(.{
+                            .data = null,
+                            .done = true,
+                            .first = false,
+                            .header = .{},
+                        }) catch {};
                         return;
                     };
+
                     self.request.redirectAsync(redirect, self.loop, self.handler) catch |err| {
                         self.handleError("Setup async redirect", err);
                         return;
@@ -1116,22 +1140,22 @@ fn AsyncHandler(comptime H: type, comptime L: type) type {
                             self.handler.onHttpResponse(.{
                                 .data = chunk,
                                 .first = first,
-                                .done = next == null,
+                                .done = false,
                                 .header = reader.response,
-                            }) catch return .done;
+                            }) catch return .handler_error;
 
                             first = false;
                         }
                     }
-                } else if (result.data != null or done or would_be_first) {
+                } else if (result.data != null or would_be_first) {
                     // If we have data. Or if the request is done. Or if this is the
                     // first time we have a complete header. Emit the chunk.
                     self.handler.onHttpResponse(.{
-                        .done = done,
+                        .done = false,
                         .data = result.data,
                         .first = would_be_first,
                         .header = reader.response,
-                    }) catch return .done;
+                    }) catch return .handler_error;
                 }
 
                 if (done == true) {
@@ -3135,7 +3159,8 @@ const CaptureHandler = struct {
         const progress = try progress_;
         const allocator = self.response.arena.allocator();
         try self.response.body.appendSlice(allocator, progress.data orelse "");
-        if (progress.done) {
+        if (progress.first) {
+            std.debug.assert(!progress.done);
             self.response.status = progress.header.status;
             try self.response.headers.ensureTotalCapacity(allocator, progress.header.headers.items.len);
             for (progress.header.headers.items) |header| {
@@ -3144,6 +3169,9 @@ const CaptureHandler = struct {
                     .value = try allocator.dupe(u8, header.value),
                 });
             }
+        }
+
+        if (progress.done) {
             self.reset.set();
         }
     }