karlseguin / websocket.zig

A websocket implementation for zig
MIT License
302 stars 28 forks source link

Discussion: Integration with http.zig #22

Closed zigster64 closed 11 months ago

zigster64 commented 11 months ago

Do you have any plans going forward to enable a simple integration of websocket.zig with http.zig ?

The common use case would be to have both servers operating on the same port, and (maybe) define the ws endpoints within the http.zig router as well.

Something like

  var server = try httpz.Server().init(allocator, .{.port = 8080});
  var router = server.router();

   router.get("/api/user/:id", getUser);
   router.websocket(Handler, &context);

    try server.listen(); 
}

I would assume this would be done in a way that doesn't affect websocket.zig - as there is still a need to write standalone WS servers that are not necessarily part of a larger web framework.

Also wouldn't be ideal to make websocket.zig a dependency in http.zig. Interesting problem - what do you reckon ?

karlseguin commented 11 months ago

I started to rework httpz's thread_pool branch to use epoll/kqueue and possibly make it fully non-blocking (well, as much as possible without forcing an event loop on the app).

This is something I can look at after that. I like the idea, but I don't want to make significant changes to accommodate it. If the change isn't too intrusive, sure. But if it makes things messy... you can front both with something, e.g. nginx, and then expose them through a single port with nginx proxy_passing to the correct one based on the location.

zigster64 commented 11 months ago

Good to hear., thank you again. No rush.

I will have a go at implementing this in httpz in the meantime, I think just looking for an upgrade handshake in the http.zig server will get most of the way there. Then it's just a matter of framing the messages. Or it could be a lot messier than that, will find out the hard way I guess. I agree that keeping it simple, free of dependencies and easy to work on is much better than the kitchen sink approach.

Unfortunately, my use case is a bit weird - I'm using this for an embedded product that ships as a single binary ... so wrapping it in nginx / caddy is not an option for me. That, and the fact that it's easy enough to just run a WS thread and a HTTP thread, on different ports. Is pretty simple really.

Keen to have a play now with the non-blocking version of http.zig !! Potential speed demon if that works out OK.

karlseguin commented 11 months ago

How would you expect it to interact with the thread_pool configuration?

Would you expect the websocket connection to execute within a thread in the pool, or is it ok if it launches a new thread per upgraded connection?

zigster64 commented 11 months ago

Hmmm … good point.

Definitely wouldn’t want websocket threads to use up the slots in the regular pool. The pool should remain free to handle high frequency short requests.

Since web socket connections (should be) infrequent and long lasting, there wouldn’t be a great benefit in them having their own pool, as the overhead in spawning a new thread is relatively small.

Would be a good idea though to define a max-websockets config value. (0 == unlimited?)

tricky bit is that all connections start life as a handler from the thread pool, with websockets spawning another thread on upgrade.

karlseguin commented 11 months ago

The blocking branch of http.zig (which was previously the master branch, and what I assume you're tracking), can now integrate with websocket.zig:

https://github.com/karlseguin/http.zig/tree/blocking#websocket

There's a new websocket configuration field that is a subset of websocket.Config for tweaking. The only real difference is that your Handler doesn't get a websocket.Handshake - you're expected to validate there request within the httpz action.

zigster64 commented 11 months ago

Excellent, many thanks. Will experiment over the next 2 weeks with this, plus hammer the other non-blocking implementation too.

you are a coding machine !

zigster64 commented 11 months ago

Converted my app from using a separate WS thread + websocket.zig -> using http.zig on a single port. This is good.

Relatively painless conversion.

The only tricky bit was that on some of the WS entry functions, I needed to create a new context on the fly, wrapping the original ctx with some auth info pulled from the original req.

Was tempted to allocate a new one, and free it on socket close. But that is not needed.

After a bit of fiddling around, found that it's simple (and safe) to just create a new context on the stack, and pass that by value.

This sort of thing works fine :

httpz.ServerCtx(*Context, *Context).init( ...,  &my_context);
router.get("/some_url/:user_id", upgrade);
...

pub fn upgrade(ctx: *Context, req: *httpz.Request, res: *httpz.Response) !void {
   .. parse the request, get various auth info

   // create a new wrapped context on the stack
    const user_ctx = QualifiedUserContext{
        .user_id = user_id,
       .token = token,
       .context = ctx,
    };

   // and pass it by value
   try httpz.upgradeWebsocket(Handler, req, res, user_ctx);

This is perfect, I really can't think of anything else to add / improve the websocket integration.

Can mark this as closed if you like.

karlseguin commented 11 months ago

Gonna keep it open until http.zig master is integrated. Should be pretty easy, but I wasn't able to write unit tests in blocking, so I'm reworking the websocket.zig tests first.

karlseguin commented 11 months ago

The master branch of http.zig now has the same integration.

Since you said you were testing the nonblocking implementation, you might want to grab the latest, since I fixed two bugs...one of which you almost certainly would have seen - seemingly random closing of the socket without sending the response.