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

🐛 BUG: Durable Objects: ws argument in webSocketClose() handler is invalid #2299

Closed
majames opened this issue Jun 19, 2024 · 4 comments
Closed
Labels
bug Something isn't working

Comments

@majames
Copy link

majames commented Jun 19, 2024

Which Cloudflare product(s) does this pertain to?

Other

What version(s) of the tool(s) are you using?

wrangler@npm:3.60.3, @cloudflare/next-on-pages@npm:1.11.3

What version of Node are you using?

20.13.1

What operating system and version are you using?

MacOS

Describe the Bug

Observed behavior

In development when I log the ws passed to webSocketClose the result is:

WebSocket { extensions: '', protocol: '', url: null, readyState: 2 }

Expected behavior

The ws passed to the close handler to have the correct url.

Steps to reproduce

  async webSocketClose(ws: WebSocket) {
    console.log(ws);
  }

At this stage it's unclear to me whether this is also an issue when in prod (i.e. running on cloudflare)

@majames majames added the bug Something isn't working label Jun 19, 2024
@Anthony291985

This comment was marked as duplicate.

@threepointone
Copy link

This is a tricky one. The WebSocket standard was based on the idea that it would be used from a browser, created with new WebSocket(url), However, inside durable objects, they're created with new WebSocketPair() to connect an incoming connection with itself, and doesn't have a URL per se (or doesn't have to be "truthful" about the incoming connection url, since you can use an URL to connect to a DO via .fetch()).

If you need to attach a url to the websocket (and have it persist through hibernation), a workaround here could be to use .serializeAttachment() to attach some state.

I'll transfer this to the workerd repo since that's where any changes would be made, let's see what the team says there.

@threepointone threepointone pinned this issue Jun 20, 2024
@threepointone threepointone unpinned this issue Jun 20, 2024
@threepointone threepointone transferred this issue from cloudflare/workers-sdk Jun 20, 2024
@MellowYarker
Copy link
Contributor

I'm inclined to agree with @threepointone. It's not really clear when we would set it on the server side with new WebSocketPair() and the URL you use to connect to a Durable Object doesn't need to be the same as the URL used to connect to the Worker. Using .serializeAttachment() sounds like the right approach here.

@majames
Copy link
Author

majames commented Jun 20, 2024

Thank you both for your clear responses. In my case I was able to retrieve the data I needed from the associated websocket via getTags().

The serializeAttachment() also sounds like a great approach if i didn't already have the required data in a tag. Thanks!

@majames majames closed this as completed Jun 20, 2024
@github-project-automation github-project-automation bot moved this from Untriaged to Done in workers-sdk Jun 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

No branches or pull requests

4 participants