karlseguin / http.zig

An HTTP/1.1 server for zig
MIT License
454 stars 31 forks source link

use config to define an optional Thread pool #21

Closed zigster64 closed 10 months ago

zigster64 commented 10 months ago

Using an extra config var to define an optional thread pool in http.zig

if set to 0, then it threads as per normal ... no change in behaviour

If set to > 0, then it uses std.Thread.Pool set to that size to have a limited pool of threads. Result is that memory consumption is greatly reduced, and remains flat over a long period. (there might be a problem with thread spawn + detach in zig stdlib, but I cant prove it yet)

See discussion on zig discord - search in help topic for "std.Thread.detach() - possible resource leak ?"

karlseguin commented 10 months ago

Gimme a day or two, but at first glance, looks fine

zigster64 commented 10 months ago

No rush. I’m running it in prod, and all seems fine with it so far.

karlseguin commented 10 months ago

This is on master, but it made me realize that std.Thread.Pool isn't ideal.

First, there's unnecessary overhead from std.Thread.Pool's ability to run anything. For httpz, we just need it to run the handleConnection load.

Second, there's now locking in the thread pool and in the ReqResPair pool. If you're going to use a thread pool, you can just give each works its own req/res/arena...you'd never need more or less ReqRes than you have worker threads.

Finally, it almost seems like a mistake that std.Thread.Pool is a LIFO. For an http server, it means old requests wait to be processed as long as new requests are coming in.

More generally, my issue with thread pools within httpz was always that it's trivially easy to DOS. With keepalive even a correctly behaving client could easily consume all the workers. Admittedly, the current behavior of just spawning more threads is pretty easy to DOS too.

I have a new branch which uses a custom thread pool exclusively. Rather than a SinglyLinkedList (LIFO) it's using a DoublyLinkedList (FIFO). There is no longer a req/res pool since every thread pool worker gets its own req/res/arena. In my testing, it's considerably faster.

HOWEVER, it's still trivially easy to DOS. So my next step is to add new configurations:

Some of these are going to be harder to implement, but I think something reasonably accurate is possible.

You can put them all to "unlimited" (the current behavior) if you have a well-configured proxy infront of httpz which would enforce all of this anyways.