Flying-Toast / simple-websockets

MIT License
18 stars 7 forks source link

Added launch_from_std_listener to support prividing own std::net::TcpListener #20

Closed StephanMoeller closed 1 year ago

StephanMoeller commented 1 year ago

Purpose is for tests to specify a listener that did not have a specific port, but got one assigned from the OS automatically. This makes the tests easier to maintain and less risk of failing to bind.

StephanMoeller commented 1 year ago

in both launch(...) and launch_from_std_listener it is now ensured, that any bind errors will populate back to the caller as the bind now happens before offloading remaining work to other thread/tasks.

This is technically a breaking change, but I would say this is a bug fix that we can now get the bind error.

It would be interesting to see if we could now remove the 500ms wait in each tests after the bind as the race condition from earlier should be gone now.

I could add some tests ensuring that bind errors in both launch-functions will actually happen as expected. Should I do that?

StephanMoeller commented 1 year ago

The tests have become unstable after this introduction. Running them again and again sometimes results in errors. I will add the new bind test, but don't accept the PR just yet. I will go dive into this as soon as I can.

StephanMoeller commented 1 year ago

The tests have now be stabilized.

Flying-Toast commented 1 year ago

This is looking great! Could we add an example to the docs for the launch_from_std_listener function? Just to make it clear that it needs to be bound before the function is called. I’m also thinking we could shorten the name to just launch_from_listener, as the type signature makes it clear that it is an std listener.

StephanMoeller commented 1 year ago

Regarding the name, that's completely up to you. I was leaving space for a launch_from_tokio_listener in the future, but I guess still in that case, that "launch_from_listener" still makes sense for the standard-listener.

StephanMoeller commented 1 year ago

Regarding the docs: Do you prefer

A) an outcommented version like this:

fn main() {
    // listen for WebSockets on port 8080:
    let event_hub = simple_websockets::launch(8080)
        .expect("failed to listen on port 8080");

    // Example of using a tcpListener:
    // let listener = TcpListener::bind(format!("0.0.0.0:8080")).unwrap();
    // let websocket_event_hub = simple_websockets::launch_from_listener(listener).expect("failed to listen on port 8080");

    // map between client ids and the client's `Responder`:
    let mut clients: HashMap<u64, Responder> = HashMap::new();

    loop {
        match event_hub.poll_event() {
            Event::Connect(client_id, responder) => {
                println!("A client connected with id #{}", client_id);
                // add their Responder to our `clients` map:
                clients.insert(client_id, responder);
            },
            Event::Disconnect(client_id) => {
                println!("Client #{} disconnected.", client_id);
                // remove the disconnected client from the clients map:
                clients.remove(&client_id);
            },
            Event::Message(client_id, message) => {
                println!("Received a message from client #{}: {:?}", client_id, message);
                // retrieve this client's `Responder`:
                let responder = clients.get(&client_id).unwrap();
                // echo the message back:
                responder.send(message);
            },
        }
    }
}

B) an entire example below the existing one with entire main and all logic replicated? C) A section below the existing example with just the two lines and some ... dots before and after like this?

fn main() {
    // listen for WebSockets on port 8080:
    let listener = TcpListener::bind(format!("0.0.0.0:8080")).unwrap();
    let websocket_event_hub = simple_websockets::launch_from_listener(listener).expect("failed to listen on port 8080");

    ...
}
Flying-Toast commented 1 year ago

C) is good.

StephanMoeller commented 1 year ago

Done :)

Flying-Toast commented 1 year ago

Thank you!

StephanMoeller commented 1 year ago

What IDE are you using? I am using vs code and having a struggle with the autoformatter which wants to put everything on a new line.