fixed endpoint example: jsonification race condition

examples/endpoint/endpoint.zig

@@ -65,31 +65,11 @@ fn getUser(e: *zap.SimpleEndpoint, r: zap.SimpleRequest) void {
 fn listUsers(e: *zap.SimpleEndpoint, r: zap.SimpleRequest) void {
     _ = e;
 
-    // 1MB json buffer
-    var jsonbuf: [1024 * 1024]u8 = undefined;
-
-    var l: std.ArrayList(User) = std.ArrayList(User).init(alloc);
-    if (users.list(&l)) {} else |_| {
-        return;
-    }
-    var maybe_json: ?[]const u8 = null;
-    var maybe_free: ?std.ArrayList(u8) = null;
-    // if (users.count > 0) {
-    if (users.count > 6000) {
-        // if > 6000 users, 1MB might not be enough for json
-        // ca 45 bytes of JSON overhead + 2 * 64 bytes of data
-        if (zap.stringifyArrayListAlloc(alloc, User, &l, .{}) catch null) |string| {
-            maybe_free = string;
-            maybe_json = string.items;
-        }
-    } else {
-        maybe_json = zap.stringifyArrayListBuf(&jsonbuf, User, &l, .{}) catch null;
-    }
-    if (maybe_json) |json| {
+    if (users.toJSON()) |json| {
         _ = r.sendJson(json);
-    }
-    if (maybe_free) |free| {
-        free.deinit();
+        alloc.free(json);
+    } else |err| {
+        std.debug.print("LIST error: {}\n", .{err});
     }
 }
 
@@ -105,7 +85,8 @@ fn postUser(e: *zap.SimpleEndpoint, r: zap.SimpleRequest) void {
                 if (zap.stringifyBuf(&jsonbuf, .{ .status = "OK", .id = id }, .{})) |json| {
                     _ = r.sendJson(json);
                 }
-            } else |_| {
+            } else |err| {
+                std.debug.print("ADDING error: {}\n", .{err});
                 return;
             }
         }

examples/endpoint/main.zig

@@ -7,6 +7,7 @@ pub fn main() !void {
         .thread_safe = true,
     }){};
     var allocator = gpa.allocator();
+
     // setup listener
     var listener = zap.SimpleEndpointListener.init(
         allocator,
@@ -14,7 +15,9 @@ pub fn main() !void {
             .port = 3000,
             .on_request = null,
             .log = true,
-            .public_folder = "./examples/endpoint/html",
+            .public_folder = "examples/endpoint/html",
+            .max_clients = 100000,
+            .max_body_size = 100 * 1024 * 1024,
         },
     );
 
@@ -35,7 +38,7 @@ pub fn main() !void {
 
     // and run
     zap.start(.{
-        .threads = 2,
-        .workers = 1, // to stay in-process, users list shared between threads
+        .threads = 2000,
+        .workers = 1,
     });
 }

examples/endpoint/users.zig

@@ -1,4 +1,5 @@
 const std = @import("std");
+const zap = @import("zap");
 
 alloc: std.mem.Allocator = undefined,
 users: std.AutoHashMap(usize, InternalUser) = undefined,
@@ -35,10 +36,12 @@ pub fn addByName(self: *Self, first: ?[]const u8, last: ?[]const u8) !usize {
     var user: InternalUser = undefined;
     user.firstnamelen = 0;
     user.lastnamelen = 0;
+
     if (first) |firstname| {
         std.mem.copy(u8, user.firstnamebuf[0..], firstname);
         user.firstnamelen = firstname.len;
     }
+
     if (last) |lastname| {
         std.mem.copy(u8, user.lastnamebuf[0..], lastname);
         user.lastnamelen = lastname.len;
@@ -49,15 +52,26 @@ pub fn addByName(self: *Self, first: ?[]const u8, last: ?[]const u8) !usize {
     defer self.lock.unlock();
     self.count = self.count + 1;
     user.id = self.count;
-    try self.users.put(user.id, user);
-    return user.id;
+    if (self.users.put(user.id, user)) {
+        return user.id;
+    } else |err| {
+        self.count -= 1;
+        std.debug.print("addByName error: {}\n", .{err});
+        // make sure we pass on the error
+        return err;
+    }
 }
 
 pub fn delete(self: *Self, id: usize) bool {
     // We lock only on insertion, deletion, and listing
     self.lock.lock();
     defer self.lock.unlock();
-    return self.users.remove(id);
+
+    const ret = self.users.remove(id);
+    if (ret) {
+        self.count -= 1;
+    }
+    return ret;
 }
 
 pub fn get(self: *Self, id: usize) ?User {
@@ -80,6 +94,7 @@ pub fn update(
     last: ?[]const u8,
 ) bool {
     // we don't care about locking here
+    // we update in-place, via getPtr
     if (self.users.getPtr(id)) |pUser| {
         pUser.firstnamelen = 0;
         pUser.lastnamelen = 0;
@@ -95,18 +110,61 @@ pub fn update(
     return false;
 }
 
-// populate the list
-pub fn list(self: *Self, out: *std.ArrayList(User)) !void {
-    // We lock only on insertion, deletion, and listing
+pub fn toJSON(self: *Self) ![]const u8 {
     self.lock.lock();
     defer self.lock.unlock();
-    var it = JsonUserIterator.init(&self.users);
+
+    // We create a User list that's JSON-friendly
+    // NOTE: we could also implement the whole JSON writing ourselves here,
+    // working directly with InternalUser elements of the users hashmap.
+    // might actually save some memory
+    // TODO: maybe do it directly with the user.items
+    var l: std.ArrayList(User) = std.ArrayList(User).init(self.alloc);
+    defer l.deinit();
+
+    var it = JsonUserIteratorWithRaceCondition.init(&self.users);
+    while (it.next()) |user| {
+        try l.append(user);
+    }
+    std.debug.assert(self.users.count() == l.items.len);
+    std.debug.assert(self.count == l.items.len);
+    return zap.stringifyArrayListAlloc(self.alloc, User, &l, .{});
+}
+
+//
+// Note: the following code is kept in here because it taught us a lesson
+//
+pub fn listWithRaceCondition(self: *Self, out: *std.ArrayList(User)) !void {
+    // We lock only on insertion, deletion, and listing
+    //
+    // NOTE: race condition:
+    // =====================
+    //
+    // the list returned from here contains elements whose slice fields
+    // (.first_name and .last_name) point to char buffers of elements of the
+    // users list:
+    //
+    // user.first_name -> internal_user.firstnamebuf[..]
+    //
+    // -> we're only referencing the memory of first and last names.
+    // -> while the caller works with this list, e.g. "slowly" converting it to
+    //    JSON, the users hashmap might be added to massively in the background,
+    //    causing it to GROW -> realloc -> all slices get invalidated!
+    //
+    // So, to mitigate that, either:
+    // - listing and converting to JSON must become one locked operation
+    // - or: the iterator must make copies of the strings
+    self.lock.lock();
+    defer self.lock.unlock();
+    var it = JsonUserIteratorWithRaceCondition.init(&self.users);
     while (it.next()) |user| {
         try out.append(user);
     }
+    std.debug.assert(self.users.count() == out.items.len);
+    std.debug.assert(self.count == out.items.len);
 }
 
-const JsonUserIterator = struct {
+const JsonUserIteratorWithRaceCondition = struct {
     it: std.AutoHashMap(usize, InternalUser).ValueIterator = undefined,
     const This = @This();
 
@@ -121,11 +179,23 @@ const JsonUserIterator = struct {
 
     pub fn next(this: *This) ?User {
         if (this.it.next()) |pUser| {
-            return User{
-                .id = pUser.id,
-                .first_name = pUser.firstnamebuf[0..pUser.firstnamelen],
-                .last_name = pUser.lastnamebuf[0..pUser.lastnamelen],
+            // we get a pointer to the internal user. so it should be safe to
+            // create slices from its first and last name buffers
+            //
+            // SEE ABOVE NOTE regarding race condition why this is can be problematic
+            var user: User = .{
+                // we don't need .* syntax but want to make it obvious
+                .id = pUser.*.id,
+                .first_name = pUser.*.firstnamebuf[0..pUser.*.firstnamelen],
+                .last_name = pUser.*.lastnamebuf[0..pUser.*.lastnamelen],
             };
+            if (pUser.*.firstnamelen == 0) {
+                user.first_name = "";
+            }
+            if (pUser.*.lastnamelen == 0) {
+                user.last_name = "";
+            }
+            return user;
         }
         return null;
     }