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.

Author: karlseguin

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

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();
         }
     }