Serve Cache from Next Client Tick#2506
Conversation
| // errdefer will handle cleanup (loop_owned is still false). | ||
| try serveFromCache(req, &cached); | ||
| transfer.deinit(); | ||
| const CacheServeCtx = struct { |
There was a problem hiding this comment.
Is there a reason to wrap this?
| ctx.* = .{ .cached = cached }; | ||
|
|
||
| try transfer.client.runNextTick(transfer.id, ctx, struct { | ||
| fn run(t: *Transfer, ctx_ptr: *anyopaque) anyerror!void { |
| const ctx = try arena.create(CacheServeCtx); | ||
| ctx.* = .{ .cached = cached }; | ||
|
|
||
| try transfer.client.runNextTick(transfer.id, ctx, struct { |
There was a problem hiding this comment.
The fd is leaked if the transfer is externally aborted before the next tick (I think)
| } | ||
|
|
||
| pub fn runNextTick(self: *Client, transfer_id: u32, ctx: *anyopaque, run: NextTickNode.Run) !void { | ||
| const node = try self.allocator.create(NextTickNode); |
There was a problem hiding this comment.
Might be worth having a MemoryPool on the HttpClient for these. Not a huge deal though
| defer self.next_tick_count -= 1; | ||
|
|
||
| if (self.findTransfer(n.transfer_id)) |t| { | ||
| try n.run(t, n.ctx); |
There was a problem hiding this comment.
If this fails, everything after it in the list is lost.
I was going to comment that, if you're going to do self.next_tick_queue = .{}; then you can avoid the overhead of popFirst() and just walk the list without removing it.
BUT..if you keep popFirst and remove self.next_tick_queue = .{}; then the 'everything is lost' is fixed.
The only issue is if drainNextTickQueue is called while drainNextTickQueue is running. Your current code handles that fine, at the cost of leaking on error.
Not sure if there's a 3rd solution that has neither drawbacks.
Also, related, but if this fails with something after it, next_tick_queue is empty, but next_tick_count > 0
| // Any concurrent CDP lookup by id will now see this transfer as gone. | ||
| _ = self.client.transfers.remove(self.id); | ||
|
|
||
| self.client.cancelNextTick(self.id); |
There was a problem hiding this comment.
It's a shame we have to walk the list on every transfer deinit.
cancelNextTick could short-circuit if next_tick_count == 0. Even better, store tick_node: ?std.DoublyLinkedList.Node on Transfer.
In fact, I'd go even further and store everything you need for this on Transfer, avoiding allocations, the transfer_id lookup, the transfer.deinit list walk.
2 participants
No description provided.