air-verse / air

☁️ Live reload for Go apps
GNU General Public License v3.0
16.32k stars 770 forks source link

Proxy doesn't forward websocket connections/messages #591

Closed nathan-hello closed 1 month ago

nathan-hello commented 1 month ago

I tried using air's proxy feature but my website uses websockets. These websockets are send by the client using HTMX's websocket extension and on the server I'm using gorilla/websocket to parse and do work on them.

When I connect to the air proxy, it doesn't seem like my websocket connection is getting through at all. Clients can't connect or receive messages over the proxy but they are able to without the proxy. I made a repo to reproduce the issue with as little code as possible.

https://github.com/nathan-hello/air-proxy-reproduce

The readme of my repo is pasted here for convenience. If supporting websockets is out of scope of air's proxy feature then I will close this issue. Thank you for making it in the first place, having HMR in things other than node is a big deal.

The site

  1. Serves index.html which has an HTMX websocket form.
  2. HTMX attempts to connect to /ws-endpoint via websocket.
  3. Upon seeing this request, /ws-endpoint upgrades the request from HTTP to websocket using gorilla/websocket.
  4. New websocket messages are sent to /ws-endpoint, which sees it in conn.ReadMessage().
  5. The ChatSocket function calls manager.BroadcastMessage(rendered) to send the rendered message to all clients.
  6. (Irrelevant to the issue) HTMX sees the html response and puts it in the bottom of the div of id "response".

The issue is that when Air tries to proxy the websocket connection, it will fail. In this sample repo, it will fail silently and I won't get any logging in the ChatSocket for loop (where the websocket logic is).

Sometimes I get the error: proxy failed to forward the response body, err: http: request method or response status code does not allow body

Which I assume is because the proxy is trying to parse HTTP requests, but this isn't HTTP, it's websocket.

Idea: Is it because the websocket upgrader HTTP code is 101? I would assume that status code doesn't have a body

To reproduce:

  1. Clone repo git clone https://github.com/nathan-hello/air-proxy-reproduce.git
  2. cd air-proxy-reproduce
  3. Run air. The version of air that I am currently running is v1.52.0
  4. Open localhost:8090 in two browser tabs. Neither one can send or receive messages

Expected behavior:

  1. Instead of running the program with air, use go run main.go.
  2. Now in the two browser tabs go to localhost:8080
  3. The two tabs should be able to send/receive messages to each other
xiantang commented 1 month ago

@ndajr PTAL

ndajr commented 1 month ago

Proxying websocket requests is possible but adds considerable complexity, and we'll need to add an extra dependency like gorilla. I avoided implementing the proxy with websockets and opted for SSE (server-sent events) just to keep it simple. Do we want to support this feature? I am curious how many people would be using air with a websocket app server

nathan-hello commented 1 month ago

I see three potential avenues for this that doesn't include doing all of the work of processing websocket messages. The premise of these is that we don't need to because websockets already have the two way communication, so it's not necessary to do all of the fancy stuff that you needed to do with SSE.

First, we just tell people to have an override of the websocket address in their html/js to the origin server. I would expect that if you say new Websocket("ws://localhost:8080/ws-endpoint"), it will connect even if you are on the proxy page localhost:8090. I will test this because I don't know if CORS-type stuff kicks in at this point. If it works I'll put in a documentation PR. I didn't think of this yesterday, but either way I would want to make this issue for documentation's sake.

The second idea is the same as the first, but we do the redirection on the server. We would send a 302 to the client on http requests that have an upgrade header. This solution is tricky because I'm not sure if the normal redirect header mechanisms work for websockets on the client side. Would responding to the upgrade request with a 302 be enough for the client to know how to finish the connection to the origin server?

Barring redirecting the clients in this way, the idea that would be a middle ground of doing some stuff in the runner/proxy.go. Create a handler that triggers on the upgrade header, connects to the origin server at whatever route over a raw TCP connection and proxies with barebones TCP data. Again, Websockets don't require the same automatic reloading in the way that you made it for http with SSE, so doing a more simplified proxy should work.

ndajr commented 1 month ago

so it's not necessary to do all of the fancy stuff that you needed to do with SSE.

It's the other way around, with SSE I was able to do in a few lines of code and no dependency was required. Now count all the lines of https://github.com/gorilla/websocket required to make it work. Adding a dependency isn't for free either, maintenance, compatibility check with new Go versions, security vulnerabilities to take care (see https://ubuntu.com/security/notices/USN-6208-1). It's a full blown protocol that should be used when a bidirectional communication is needed, which is not the case of live reloading. If you know a way of proxying websocket requests without implementing a websocket server on the proxy, feel free to create a PR

nathan-hello commented 1 month ago

Forcing the websocket connection url to be from ws://localhost:8080/ws-endpoint seems to work fine. I'm a little embarrassed I didn't think of this sooner. Issue is closed.

Thanks again for the feature!