karlseguin / websocket.zig

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

Listening to server disconnection/Inavailability without panicing the client #18

Closed hajsf closed 10 months ago

hajsf commented 11 months ago

In my client file I wrote:

const Handler = struct {
    client: websocket.Client,

    pub fn init(allocator: std.mem.Allocator, host: []const u8, port: u16) !Handler {
        return .{
            .client = try websocket.connect(allocator, host, port, .{}),
        };
    }

    pub fn deinit(self: *Handler) void {
        self.client.deinit();
    }

    pub fn connect(self: *Handler, path: []const u8) !void {
        try self.client.handshake(path, .{.timeout_ms = 5000});
        const thread = try self.client.readLoopInNewThread(self);
        thread.detach();
    }

    pub fn handle(_: Handler, message: websocket.Message) !void {
        const data = message.data;
        std.debug.print("CLIENT GOT: {s}\n", .{data});
    }

    pub fn write(self: *Handler, data: []u8) !void {
        return self.client.write(data);
    }

    pub fn close(_: Handler) void {
        std.log.err("Server socket had been closed", .{});
    }
};

pub fn main() !void {
    var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = general_purpose_allocator.allocator();

    var handler = try Handler.init(allocator, "127.0.0.1", 9223);
    defer handler.deinit();
    // spins up a thread to listen to new messages
    try handler.connect("/");

    var data = try allocator.dupe(u8, "hello world");
    try handler.write(data);

    data = try allocator.dupe(u8, "hello Zig");
    try handler.write(data);
    data = try allocator.dupe(u8, "hello Hasan");
    try handler.write(data);

    // without this, we'll exit immediately without having time to receive the
    // echo'd message from the server
    std.time.sleep(std.time.ns_per_s);
}

If I pressed Ctrl+C and closed the server, then I get the below:

  1. The close function is executed
  2. The program got panic:
    
    error: Server socket had been closed
    error.Unexpected: GetLastError(64): The specified network name is no longer available.

C:\zig-windows-x86_64-0.11.0\zig\lib\std\os\windows.zig:630:53: 0x7ff64b537547 in WriteFile (ws.exe.obj) else => |err| return unexpectedError(err), ^ C:\zig-windows-x86_64-0.11.0\zig\lib\std\net.zig:1814:40: 0x7ff64b4cb3a7 in write (ws.exe.obj) return os.windows.WriteFile(self.handle, buffer, null, io.default_mode); ^ C:\zig-windows-x86_64-0.11.0\zig\lib\std\net.zig:1827:36: 0x7ff64b493865 in writeAll (ws.exe.obj) index += try self.write(bytes[index..]); ^ D:\ubuntu\zig\websocket.zig\src\client.zig:276:30: 0x7ff64b4936cc in writeAll (ws.exe.obj) return self.stream.writeAll(data); ^ D:\ubuntu\zig\websocket.zig\src\client.zig:200:24: 0x7ff64b4a7069 in writeFrame (ws.exe.obj) try stream.writeAll(buf[0..6]); self.writeFrame(.close, "") catch {}; ^ D:\ubuntu\zig\websocket.zig\src\client.zig:79:14: 0x7ff64b4d8876 in deinit (ws.exe.obj) self.close(); ^ D:\ubuntu\zig\ws.zig:14:27: 0x7ff64b4a7cfa in deinit (ws.exe.obj) self.client.deinit(); ^ D:\ubuntu\zig\ws.zig:43:25: 0x7ff64b4a7cad in main (ws.exe.obj) defer handler.deinit(); ^ C:\zig-windows-x86_64-0.11.0\zig\lib\std\start.zig:339:65: 0x7ff64b4a7dbc in WinStartup (ws.exe.obj)
std.os.windows.kernel32.ExitProcess(initEventLoopAndCallMain()); ^ ???:?:?: 0x7ffc24f9257c in ??? (KERNEL32.DLL) ???:?:?: 0x7ffc264eaa67 in ??? (ntdll.dll)


Is there a way that I can listen to this even without panic, can I capture the `error.Unexpected: GetLastError(64): The specified network name is no longer available.` error and handle it, and keep the client running?

Same is applicable in case the client is trying to connect to the server but found it unavailble, it is giving err `error: ConnectionRefused` and panic;
```bach
error: ConnectionRefused
C:\zig-windows-x86_64-0.11.0\zig\lib\std\net.zig:722:5: 0x7ff687311a4e in tcpConnectToHost (ws.exe.obj)
    return std.os.ConnectError.ConnectionRefused;
    ^
D:\ubuntu\zig\websocket.zig\src\client.zig:33:21: 0x7ff687311669 in connect (ws.exe.obj)
 const net_stream = try net.tcpConnectToHost(allocator, host, port);
                    ^
D:\ubuntu\zig\ws.zig:9:23: 0x7ff6873bdc2f in init (ws.exe.obj)
            .client = try websocket.connect(allocator, host, port, .{}),
                      ^
D:\ubuntu\zig\ws.zig:42:19: 0x7ff6873d79dc in main (ws.exe.obj)
    var handler = try Handler.init(allocator, "127.0.0.1", 9223);
                  ^
karlseguin commented 11 months ago

For the failing on connect, that's your own code. Replace the try Handler.init with something like:

Handler.init(allocator, "127.0.0.1", 9223) catch |err| switch (err) {
   error.ConnectionRefused => {....}
   else => // TODO
}

The websocket.connect bubbles up errors to the application to handle. By using try in your Handle.init you're bubbling it up to your main. And in main you use try, which exits the program.

The close error is a bug in Zig itself. You can find the issue https://github.com/ziglang/zig/issues/15702

Fix looks simple, but figuring out how to build/test Zig for windows doesn't :(

hajsf commented 11 months ago
else => // TODO

mm, how can I replace defer handler.deinit(); inside this block?

karlseguin commented 11 months ago

Properly written code should not need cleanup on failure. And if it does, I'd expect it to be very well documented.

So if Handler.init returns an error, should not be called (you have nothing to call it on anyways):

const handler = Handler.init(allocator, "127.0.0.1", 9223) catch |err| switch (err) {
   error.ConnectionRefused => {....}
   else => // TODO
}
defer handler.deinit();

If your Handler.init is complicated with multiple allocations...then you use errdefer within it, something like:

pub fn init(allocator: Allocator, host: []const u8, port: u16) !Handler {
    const buf = try allocator.alloc(u8, 1024);
    errdefer allocator.free(buf);

    const client = try websocket.connect(allocator, host, port, .{});
    // this isn't strictly necessary, since nothing else can fail in this function
    // but it's nice to have in case you add more logic to this function that CAN
    // fail
    errdefer client.deinit();

    return .{
        .buf = buf,
        .client = client,
    };
}

So you can see from the above funciton, init either returns succesful, in which case the caller will need to call its deinit() at some point, or it fails, in which case there's nothing to cleanup because init cleaned itself up (via errdefer)

hajsf commented 11 months ago

Thanks, do you think the below sounds fine in the init and main:

const std = @import("std");
const websocket = @import("./websocket.zig/src/websocket.zig");

const Handler = struct {
    client: websocket.Client,

    pub fn init(allocator: std.mem.Allocator, host: []const u8, port: u16, handler: *Handler) anyerror!void { // error{ConnectionRefused}!void
        std.log.info("connected 1", .{});

        const client = try websocket.connect(allocator, host, port, .{});

        errdefer client.deinit();

        handler.* = .{
            .client = client,
        };
    }
    pub fn deinit(self: *Handler) void {
        std.log.info("connected 2", .{});
        self.client.deinit();
    }

    pub fn connect(self: *Handler, path: []const u8) !void {
        std.log.info("connected 3", .{});
        try self.client.handshake(path, .{ .timeout_ms = 5000 });
        const thread = try self.client.readLoopInNewThread(self);
        thread.detach();
    }

    pub fn handle(_: Handler, message: websocket.Message) !void {
        std.log.info("handle", .{});
        const data = message.data;
        std.debug.print("CLIENT GOT: {s}\n", .{data});
    }

    pub fn write(self: *Handler, data: []u8) !void {
        std.log.info("connected 4", .{});
        return self.client.write(data);
    }

    pub fn close(_: Handler) void {
        std.log.info("connected 5", .{});
        std.log.err("Server socket had been closed", .{});
    }
};

pub fn main() !void {
    var general_purpose_allocator = std.heap.GeneralPurposeAllocator(.{}){};
    const allocator = general_purpose_allocator.allocator();

    var handler: Handler = undefined;
    Handler.init(allocator, "127.0.0.1", 9223, &handler) catch |e| {
        std.log.err("Error initializing handler: {}", .{e});
        return;
    };

    defer handler.deinit();

    // spins up a thread to listen to new messages
    try handler.connect("/");

    var data = try allocator.dupe(u8, "hello world");
    try handler.write(data);

    data = try allocator.dupe(u8, "hello Zig");
    try handler.write(data);
    data = try allocator.dupe(u8, "hello Hasan");
    try handler.write(data);

    // without this, we'll exit immediately without having time to receive the
    // echo'd message from the server
    std.time.sleep(std.time.ns_per_s);
}

I got the output as:

PS D:\ubuntu\zig> zig run ws.zig
info: connected 1
error: Error initializing handler: error.ConnectionRefused
PS D:\ubuntu\zig> 
karlseguin commented 11 months ago

That's the expected output? or an error?

I think the code is fine. I find it a bit strange for init to be written like that. Not sure why you opted for that instead of:

pub fn init(allocator: std.mem.Allocator, host: []const u8, port: u16) !Handler { 
    std.log.info("connected 1", .{});

    const client = try websocket.connect(allocator, host, port, .{});
    // not strictly necessary here, but would be if you add more code after that can fail
    errdefer client.deinit();

    return .{
        .client = client,
    };
}

And then use var handler = Handler.init(....) catch ...

Also, it's better to use an implicit error said, i.e. !Handler, instead of anyerror. anyerror is a is a superset of all possible errors in the program. The implicit error set will be specifically for the errors the function can return.

hajsf commented 11 months ago

That's the expected output? or an error?

It is the expected output