karlseguin / websocket.zig

A websocket implementation for zig
MIT License
283 stars 25 forks source link

writeCloseWithReason doesn't produce a reason and status code in the browser #41

Closed scoreboardxdev closed 1 month ago

scoreboardxdev commented 1 month ago

It seems like writeCloseWithReason does not work properly. In the browser, I receive a 1006 status code without a reason, while in Node.js, using the ws package, I receive the reason and status properly. The connection is established properly, and I have received the handshake on the server side.

karlseguin commented 1 month ago

I'm seeing it work fine. In wireshark, I can see the close with reason.

I also took this Golang exmaple: https://raw.githubusercontent.com/gorilla/websocket/main/examples/echo/client.go (and changed the path and port)

And had my Zig handler do:

    pub fn handle(self: *Handler, message: Message) !void {
        // const data = message.data;
        _ = message;
        try self.conn.writeCloseWithReason(4000, "bye bye");
        self.conn.close();
        // try self.conn.write(data); // echo the message back
    }

Got this:

connecting to ws://localhost:9223/
read: websocket: close 4000: bye bye

FWIW, 1006 is reserved so I'm not sure if that's messing anything up (websocket.zig doesn't enforce it though)

1006 is a reserved value and MUST NOT be set as a status code in a Close control frame by an endpoint

scoreboardxdev commented 1 month ago

I checked your code, and you are right. I forgot to call close after writing. What do you think about closing the connection automatically when writeClose/writeCloseWithReason is called?

karlseguin commented 1 month ago

When I first looked into this issue, I was also surprised by this. I guess that's why I called it writeClose.

Anyways, I'm working on significant changes which will break some of the API. It's in the nonblocking branch. I'd like to combine all breaking changes together.

In the nonblocking branch, there's only conn.close(.{}) with the default opts of: .{.code = 1000, .reason = "}. This combines both the call to writeClose() and close() AND it immediately closes the connection. This change was added to the migration wiki.