Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WebSocket support #124

Merged
merged 9 commits into from
Jan 28, 2022
Merged

WebSocket support #124

merged 9 commits into from
Jan 28, 2022

Conversation

ByteAlex
Copy link
Contributor

My initial approach to supporting websockets in workers-rs.
Sending data works fine, though receiving data - for example through onmessage - does not work yet.

I'd appreciate some help figuring this out!

Also at maintainers, would this PR, if properly documented, get an OK from you?

@ByteAlex ByteAlex marked this pull request as draft January 28, 2022 03:18
@ByteAlex
Copy link
Contributor Author

ByteAlex commented Jan 28, 2022

I did some initial investigation on why the onmessage does not work and it seems that the JS onmessage is not called at all.

export function __wbg_setonmessage_8bf4fad923b5767c(arg0, arg1) {
    console.log(new WebSocketPair()[1]);
    let ws = getObject(arg0);
    console.log(ws);
    ws.onmessage = function (evt) {
        console.log(evt);
        let fn = getObject(arg1);
        console.log(fn);
        fn(evt);
    };
};

The first log was solely to figure how the WS should look.
The second log was too see whether the WS is correctly instantiated.

These logs are written once a ws-request is received.

then I have the ws.onmessage and it's contents are never logged. So it does not seem to be an issue with the closure, but it implies that the ResponseInit did not properly carry the webSocket to the browser, I think?

Taking a look into the workers-chat-demo, I see

return new Response(null, { status: 101, webSocket: pair[0] });

Which should be the same as my rust implementation.
ResponseInit in worker-sys:

    #[doc = "Change the `webSocket` field of this object."]
    pub fn websocket(&mut self, val: &WebSocket) -> &mut Self {
        let r = ::js_sys::Reflect::set(self.as_ref(), &JsValue::from("webSocket"), val.as_ref());
        debug_assert!(
            r.is_ok(),
            "setting properties should never fail on our dictionary objects"
        );
        let _ = r;
        self
    }

But debugs of it show: The webSocket is defined in the ResponseInit:

{ status: 101, headers: HeadersList(0) [], webSocket: WebSocket {} }

@ByteAlex
Copy link
Contributor Author

I intended to go to sleep when I was sending my investigation answer, but I wanted to get this done, so I kept searching.

The issue was more stupid than I thought; It's not "set_onmessage", as implied by the generated WebSocket bindings from wasm, but set_event_handler.
After fixing those, this PR should be fully functional.
I've tested sending, receiving and echoing data aswell as the close event, but I was not able to test an "error" case yet.

@nilslice
Copy link
Contributor

@ByteAlex - wow, thank you for all of this. We will start taking a look, but please convert it to a "ready for review" PR once you think it is out of Draft.

Also at maintainers, would this PR, if properly documented, get an OK from you?

Yes, I don't see why not! @zebp 👀 🚀

@ByteAlex
Copy link
Contributor Author

@nilslice Will do! Planning to complete documentation later today (this evening) and will turn it into "Ready for Review".
I do not expect any more changes to the actual implementation, as I've been using the current version in my other project already and it seems solid!

What probably is important: I removed the web_sys::ResponseInit implementation and did an own implementation in worker_sys::response_init::ResponseInit, due to the required additional webSocket field.

@zebp
Copy link
Collaborator

zebp commented Jan 28, 2022

Also at maintainers, would this PR, if properly documented, get an OK from you?

Yes, I don't see why not! @zebp eyes rocket

Yeah this would be awesome @ByteAlex! You're making great progress and it looks pretty good so far.

Edit: Removed clippy reminder, forgot it isn't commented out anymore we just pinned wasm-bindgen instead.

aka make linter happy again
@ByteAlex ByteAlex marked this pull request as ready for review January 28, 2022 19:38
@ByteAlex
Copy link
Contributor Author

Yeah this would be awesome @ByteAlex! You're making great progress and it looks pretty good so far.

Edit: Removed clippy reminder, forgot it isn't commented out anymore we just pinned wasm-bindgen instead.

Haha yeah, Linters / Formatter is always complaining about my code, but I think it should be all good now + it's ready for review! 🚀

@ByteAlex ByteAlex changed the title WIP: WebSocket support WebSocket support Jan 28, 2022
Copy link
Collaborator

@zebp zebp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor changes, but overall this is awesome :)

worker/src/websocket.rs Outdated Show resolved Hide resolved
worker/src/websocket.rs Show resolved Hide resolved
@ByteAlex ByteAlex requested a review from zebp January 28, 2022 21:36
@zebp zebp merged commit b9b4d08 into cloudflare:main Jan 28, 2022
@ByteAlex ByteAlex deleted the websocket-support branch January 28, 2022 21:50
@zebp zebp mentioned this pull request Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants