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

Implement Hibernatable Web Sockets API #436

Merged
merged 15 commits into from
Feb 28, 2024

Conversation

DylanRJohnston
Copy link
Contributor

@DylanRJohnston DylanRJohnston commented Jan 17, 2024

Hey all, this contains my attempt at writing the bindings for the Hibernating Web Sockets API. I haven't implemented the setWebSocketAutoResponse suite of functions yet as I don't have immediate use for them and I'd like feedback on the implementation so far.

Supersedes #434 as I was unable to change the branch for the pull request and it contained more changes than just the Hibernatable web sockets API.

pub fn accept_websocket_with_tags(
this: &DurableObjectState,
ws: web_sys::WebSocket,
tags: Vec<JsValue>,
Copy link
Contributor Author

@DylanRJohnston DylanRJohnston Jan 17, 2024

Choose a reason for hiding this comment

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

wasm_bindgen v0.2.89 supports using Vec<String> directly. There's a few places in the workers-rs API surface that could benefit from this change.

@eric-seppanen
Copy link
Contributor

I did a quick functional test of this branch and it works in my project-- I am testing 2 websockets in a durable object, using binary messages and hibernation.

@@ -15,4 +15,21 @@ extern "C" {

#[wasm_bindgen(method, js_name=waitUntil)]
pub fn wait_until(this: &DurableObjectState, promise: &js_sys::Promise);

#[wasm_bindgen(method, js_name=acceptWebSocket)]
pub fn accept_websocket(this: &DurableObjectState, ws: &web_sys::WebSocket);

Choose a reason for hiding this comment

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

Calling accept() on a WebSocket in an Object will incur duration charges for the entire time the WebSocket is connected. If you prefer, use state.acceptWebSocket() instead, which will stop incurring duration charges once all event handlers finish running.

https://developers.cloudflare.com/durable-objects/platform/pricing/

The support of acceptWebSocket() is critical to many existing rust-based Durable Object applications since it would dramatically lower the cost incurred by WebSocket connection currently implemented by accept() method. Really appreciate your work!

@MellowYarker
Copy link

Hey @DylanRJohnston, I commented on the previous PR about state.getTags(ws). You previously asked which version of wrangler supported it and I believe it should be [email protected].

@eric-seppanen
Copy link
Contributor

eric-seppanen commented Feb 6, 2024

As an experiment I added getTags support to my local branch, and the following works for me:

impl State {
    /// Retrieve tags from a hibernatable websocket
    pub fn get_tags(&self, websocket: &WebSocket) -> Vec<String> {
        let tags = self.inner.get_tags(websocket.as_ref());
        let tags: js_sys::Array = tags.dyn_into().expect("get_tags returned wrong type");

        tags.iter()
            .map(|tag| tag.as_string().expect("get_tags returned non-string value"))
            .collect()
    }
}

And some wasm_bindgen glue in worker-sys state.rs:

#[wasm_bindgen(method, js_name=getTags)]
pub fn get_tags(this: &DurableObjectState, ws: &web_sys::WebSocket) -> wasm_bindgen::JsValue;

Maybe the error handling is unnecessary? Probably unchecked_into would be fine; not sure if there's an unchecked way to do as_string.

@DylanRJohnston
Copy link
Contributor Author

DylanRJohnston commented Feb 7, 2024

I think you can change the signature of

pub fn get_tags(this: &DurableObjectState, ws: &web_sys::WebSocket) -> wasm_bindgen::JsValue;

into

pub fn get_tags(this: &DurableObjectState, ws: &web_sys::WebSocket) -> Vec<String>

and have wasm_bindgen do the work of converting it for you.

@j-white
Copy link
Contributor

j-white commented Feb 26, 2024

Nice work & a big thanks for putting this together. I was able to switch over to use the Hibernatable Web Sockets API fairly easily with this patch!

Copy link

@MellowYarker MellowYarker left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @DylanRJohnston! For tracking purposes, the following Hibernation API methods are still unimplemented in this PR, but this is off to a great start:

  • getTags()
  • getWebSocketAutoResponse()
  • setWebSocketAutoResponse()
  • getWebSocketAutoResponseTimestamp()
  • getHibernatableWebSocketEventTimeout()
  • setHibernatableWebSocketEventTimeout()

My sense is these can be implemented in a later PR, although it would be nice to include getTags() here. I'm not usually in this repo, so I won't approve the PR myself, but I'll check in with the runtime team to see who can take a look at this.

);

#[wasm_bindgen(method, js_name=getWebSockets)]
pub fn get_websockets(this: &DurableObjectState) -> Vec<web_sys::WebSocket>;

Choose a reason for hiding this comment

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

Just curious, why do there need to be two versions of acceptWebSocket and getWebSockets? The JS and C++ methods all provide an optional/maybe for tags, would an Option not work here?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion (I am not the author), it's a style choice but this is probably the right one (in Rust anyway).

It's slightly kinder to the caller to have two functions, and probably more idiomatic in Rust. Passing a parameter None hurts the readability of the code, because it's not obvious at a glance what that parameter means.

The rest of this crate seems to mostly follow this style as well, e.g. ObjectNamespace::unique_id/unique_id_with_jurisdiction, Storage::list/list_with_options...

Copy link
Contributor Author

@DylanRJohnston DylanRJohnston Feb 28, 2024

Choose a reason for hiding this comment

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

Yeah it's an unfortunate impedance mismatch between function overloading in JS and the lack thereof in Rust. I'm just trying to match the style of the rest of the repo which manually "un-overloads" the functions as Eric pointed out.

I kind of wish Rust did have function overloading, but it probably has complicated interactions with the trait solver and type inference.

ws: WebSocket,
message: WebSocketIncomingMessage,
) -> Result<()> {
unimplemented!("websocket_message() handler not implemented")

Choose a reason for hiding this comment

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

So if I'm writing a DO in rust and I use websocket hibernation, I have to implement these otherwise when they're dispatched we panic? If you're aiming to match Workerds behavior we silently drop the event, but I see the value in warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree with you, however I'm just matching the existing implementation of alarm above.

#[allow(clippy::diverging_sub_expression)]
    async fn alarm(&mut self) -> Result<Response> {
        unimplemented!("alarm() handler not implemented")
    }

If you'd like this approach changed, I think we should also change the alarm implementation to match.

worker/src/env.rs Outdated Show resolved Hide resolved
worker/src/durable.rs Show resolved Hide resolved
kflansburg
kflansburg previously approved these changes Feb 28, 2024
kflansburg
kflansburg previously approved these changes Feb 28, 2024
@kflansburg kflansburg merged commit 1ddf6d7 into cloudflare:main Feb 28, 2024
3 checks passed
jdon pushed a commit to jdon/workers-rs that referenced this pull request Mar 27, 2024
* Rebase commit

* Pull Request feedback

* Fix problems with async_trait

* Improve error message around incorrect impl methods

* Improve error message around incorrect impl methods

* Add missing semi-colon

* Add missing async

* Add clippy exceptions

* Fix trait type

* Properly qualify worker_sys

* Change websockets to ref:

* Revert formatting changes to Cargo.toml

* Remove left-over code

* fix formatting

* clippy

---------

Co-authored-by: Kevin Flansburg <[email protected]>
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.

6 participants