kwhitley / itty-durable

Cloudflare Durable Objects + Itty Router = shorter code
MIT License
251 stars 18 forks source link

When trying to use websockets the connection hangs on trying to connect #3

Open garretcharp opened 3 years ago

garretcharp commented 3 years ago

I was attempting to use WebSockets and whenever using itty-durable the connection hangs on trying to connect. Below is a small little code sample of what I am using to test the WebSockets. When I use it outside the scope of itty-durable (just on a normal route) it works fine.

import { ThrowableRouter } from 'itty-router-extras'
import { createIttyDurable, withDurables } from 'itty-durable'

export class TestWebsocket extends createIttyDurable({ persistOnChange: false }) {
    constructor(state, env) {
        super(state, env)
    }

    websocket() {
        const [client, websocket] = Object.values(new WebSocketPair())

        websocket.accept()

        websocket.send(JSON.stringify({ connected: true }))

        websocket.addEventListener('message', ({ data }) => {
            websocket.send(JSON.stringify({ data }))
        })

        return new Response(null, { status: 101, webSocket: client })
    }
}

const router = ThrowableRouter()

router.all('*', withDurables())

// Doesnt work
router.get('/ws', ({ TestWebsocket }) => TestWebsocket.get('test').websocket())

// Works
router.get('/ws2', () => {
    const [client, websocket] = Object.values(new WebSocketPair())

    websocket.accept()

    websocket.send(JSON.stringify({ connected: true }))

    websocket.addEventListener('message', ({ data }) => {
        websocket.send(JSON.stringify({ data }))
    })

    return new Response(null, { status: 101, webSocket: client })
})

export default {
    fetch: router.handle
}
repzip commented 3 years ago

Did you get this working?

TimTinkers commented 3 years ago

Dropping by also to see @garretcharp @repzip if either of you had found a solution to this.

garretcharp commented 3 years ago

Hey guys, I did get this working at some point I actually did it without using itty-durable and instead only using itty-router.

Basically in the DO I create an itty router and expose a fetch function that calls the router. Following code probably wont work but a general idea:

import { Router } from 'itty-router'

const router = Router()

router.all('/my/do/route/*', async (req, { MyDo }) => {
  const doId = MyDo.idFromString('<id>')
  return MyDo.get(doId).fetch(req)
})

// DO:
export class MyDo {
  constructor() {
    // do stuff

    this.router = Router()

    this.router.get('/get/ws', request => {
       const [client, websocket] = Object.values(new WebSocketPair())

    websocket.accept()

    websocket.send(JSON.stringify({ connected: true }))

    websocket.addEventListener('message', ({ data }) => {
        websocket.send(JSON.stringify({ data }))
    })

    return new Response(null, { status: 101, webSocket: client })
    })
  }

  async fetch(request) {
    return this.router.handle(request)
  }
}
TimTinkers commented 3 years ago

Hi @garretcharp, thanks for the tip. Unfortunate that this doesn't work out of the box; makes itty-durable a bit niche for our usage right now. Dropping an itty-router into the durable object was able to remove at least some of the boilerplate.

kalepail commented 2 years ago

Dug into this a bit and it would appear that there's a header issue or maybe just a response issue where the webSocket: client isn't being forwarded when responding. This is either before the pass back or actually in the pass in where the headers.Upgrade: 'websocket' isn't being passed to the method call into the DO.

kwhitley commented 2 years ago

I'm gonna circle back to this... as none of my test scenarios involved websockets. Would love to ensure support for that side of things, just need to do some testing! If anyone has an example repo that I can use as a demo/testbed, that would be a huge help!

In the meantime, the standard (non-socket) v1.x is being wrapped up today, with the final API changes and bug-fixes over the alpha version that's been languishing since launch.

da7a90-backup commented 2 years ago

Would love to contribute to adding web socket support, I feel like this is one of the most powerful and in demand features

AndrewLester commented 1 year ago

Would also like to contribute to make this happen, and before doing any work I'd like to summarize what (I think?) the challenges might be:

Going about implementing all this, forwarding all request headers through with the proxy'd function fetch request doesn't seem too difficult IF that's a sensible solution. Maybe forwarding all headers is a bad idea? I'm not sure.

With the POST vs GET issue, it seems like the proxy'd function fetches with POST are pretty ingrained in the system, so breaking out of that for WebSockets could be less pretty. Perhaps the proxyDurable function's get handler could detect the Upgrade: websocket header, and thus perform a GET request instead of a POST? This wouldn't support a request body though, which could instead be turned into URL params and be picked up by the itty-durable DO internal router.

kwhitley commented 1 year ago

I'd love the help on this one, and it's one I'll me short listing myself anyway, so it's perfect timing to get another set of eyes/ideas on tackling this. I too, have run into crash after crash while trying to obtain a socket connection from within the DO.

Personally I had assumed that it didn't even matter that the internal proxied fetches were over POST, as in theory, any function within the DO (even if originally called by POST) could respond to a Worker that originated the request from a client via the proper GET/upgrade request. As in, the Worker would intercept the upgrade request, fire a POST/proxied request to, say a "connect()" function on the DO, which would instantiate a socket pair and respond with the client, which the Worker would then pass along on back to the original client.

  • As mentioned above, the Upgrade: websocket header needs to be sent to the DO when making a proxy'd function fetch. In doing this, maybe it makes sense to forward all headers of the current request over to the DO?

I don't see any reason against doing this...

  • The proxy'd function fetch that you'd get from writing DurableObject.connect({ connectionData }), currently, is only ever a POST request. But, from what I can tell, WebSocket connections MUST be initiated with a GET request.

Like I described (poorly) above, the Worker can handle the GET/upgrade, and ideally return a socket client from the DO, but i don't think the DO should have to care about any of that... but that's an unsubstantiated guess at this point, as I haven't gotten it working yet. Client<-->Worker socket yes, Client<-->Worker<-->DO not so much.

AndrewLester commented 1 year ago

I was just trying to do some tests on that POST vs. GET thing when sending a WebSocket connection request to a DO and I found some very strange behavior. I have a repro in this GitHub repository here.

The behavior I noticed is that, if the Upgrade: websocket header is present on a request sent to the DO, the request method is automatically set to GET. This only happens on non-local development or prod, that is, if I'm running on Miniflare with --local, a POST request method is still logged. Otherwise, I can only seem to log GET. Again, I may be doing something wrong here. There's also a little bit of other code from what I was testing, so it's not exactly a minimal repro yet...

Maybe this line is actually what I was looking for.

kwhitley commented 1 year ago

Ok, so yesterday I successfully got itty-durable (modified version) to play nicely with, and return a websocket.

A few things to note:

gishmel commented 1 year ago

What's the timeline on this fix landing in this library and potentially itty-router if also relevant? Also follow on question is this equally a problem in itty-router; I had issues with itty-router seemingly not passing through the Upgrade: websocket header between the outside worker or routing worker into the DO worker and thought it is probably related to this issue but wanted to ask to understand better. Thanks for all your work; I love itty-router it's a brilliant library that makes Workers really exciting and terse to accomplish amazing things!

kwhitley commented 1 year ago

@gishmel - I just finished getting it all to work last week, so I just need to backport the itty-durable changes back into the main lib and publish, so hopefully today. No changes necessary in itty-router, which doesn't surprise me, as it's completely return-agnostic (doesn't care if anything returned is a Response or not). The issue was a combination of the following:

The good news?

This should all be 100% transparent in your own itty-durable code, requiring no changes!

gishmel commented 1 year ago

Thanks so much for the extra clarification seems like my problem with itty-router must be self inflicted I thought I was passing back the right headers at the right times but I guess somewhere things aren't propagating correctly. Did you happen to have an example repo where one could see these types of operations working successfully getting an example request from an outside worker and sending it through a Durable Object which returns a websocket client to the outside worker which returns that to the client? I can't help but think maybe I am missing something crucial for websockets specifically since I have no issues with any HTTP related actions or routes? Thanks again for all your work it's been a blast to use itty-* and I look forward to keep using them for a long time to come!

kwhitley commented 1 year ago

I'm trying to work up a simple code example for both a client-Worker socket, and a client-Worker-DO socket. It likely won't be perfect, but i'll publish in advance so you can test the functionality, even if I just need to work out a working example with you here (or on Discord)

kwhitley commented 1 year ago

But it def wasn't your fault - even if you tried to force all the right things with itty-router, the itty-durable layers were preventing any of the right headers from making it through to the DO!

kwhitley commented 1 year ago

Ok, itty-durable@next is released with these changes, so I'd suggest trying it out so I have another set of eyes on it. Unfortunately due to the nature of DO complexity paired with the multi-layer proxy magic of itty-durable, unit tests haven't been tackled like they have in every other lib, making this process a little bit trial-by-fire.

As to the example, I ported some of the example code over to examples/websocket, showing an endpoint that connects a client to a Worker, and other one that lets the DO create the socket instead. These came slightly modified/cleaned up from my working code on OTP Garden, so they hopefully work without too much issue.

router code

import { withDurables } from 'itty-durable'
import { Router } from 'itty-router'
import { error, withParams } from 'itty-router-extras'

// export durable object class, per spec
export * from './Room'

const router = Router()

router
  .all('*', withDurables())

  // GENERAL CLIENT-WORKER SOCKET EXAMPLE
  .get('/connect', (request) => {
    const [client, server] = Object.values(new WebSocketPair())

    server.accept()

    // if a client closes the connection, close it immediately
    server.addEventListener('close', () => {
      server.close()
    })

    // test the connection by sending a delayed message after 2 seconds
    setTimeout(() => {
      server.send('delayed message...')
    }, 2000)

    return new Response(null, { status: 101, websocket: client })
  })

  // CLIENT-WORKER-DO SOCKET EXAMPLE
  .get('/room/:id/connect', withParams,
    ({ id, Room }) => Room.get(id).connect()
  )

// CF ES6 module syntax
export default {
  fetch: (request, env, context) => router
                        .handle(request, env, context)
                        .catch((err) => error(err.status || 500, err.message))
}

Durable Object code

import { createDurable } from 'itty-durable'

export class Room extends createDurable({ autoPersist: true }) {
  sockets: any[]

  constructor(state, env) {
    super(state, env)

    this.sockets = []
  }

  // CREATE NEW SOCKET
  connect() {
    const { sockets } = this
    const id = Math.random() // just a random id
    const [client, server] = Object.values(new WebSocketPair())

    server.accept() // immediately accept connection

    // if a client closes the connection, close it immediately
    server.addEventListener('close', () => {
      server.close()
      this.sockets = this.sockets.filter(s => s.id !== id)
    })

    // add socket.server to list of open sockets
    sockets.push({ id, server })

    // send a message to all connected sockets
    this.sendMessage('A new connection has been established!')

    return new Response(null, { status: 101, webSocket: client })
  }

  sendMessage(message: string) {
    // send a message to all open sockets
    for (const socket of this.sockets) {
      socket.send(message)
    }
  }
}

Hope this gets you a start!

rahul-sharma-uipath commented 1 year ago

@kwhitley I was able to confirm your solution works with itty-durable@next - as expected per this thread, switching from next back to latest stable (1.6.0) causes the socket to hang again.

is there any ETA on when the changes in next will be officially released?