Request compression when calling `new WebSocket()` by default

[MellowYarker]

Browsers set the compression extension by default when new WebSocket() is called, since the API doesn't give the caller control over that header. See step 9 of whatwg's opening handshake process for websockets.

---

I think we can leave the fetch() implementation as is, since you could easily just set the extension header yourself:

  let resp = await fetch(request, {
    headers: {
      "Upgrade": "websocket",
      "Sec-WebSocket-Extensions": "permessage-deflate"
    }
  });

[bretthoerner]

I think we can leave the fetch() implementation as is, since you could easily just set the extension header yourself:

Naive question, but I figure most people will forget to do this, is there a downside to defaulting to compression (I'm assuming the server has to accept the extension, of course).

[MellowYarker]

I think we can leave the fetch() implementation as is, since you could easily just set the extension header yourself:

Naive question, but I figure most people will forget to do this, is there a downside to defaulting to compression (I'm assuming the server has to accept the extension, of course).

Yeah, the server has to accept the extension. The scariest thing to me is that we'd be going from a world where we have no compression to a world where (potentially) all websocket communication is compressed. Even if we do want to enable it by default on fetch(), we could always do that later once we've seen it work fine with new WebSocket() for some time.

Another downside would be all the extra computation required to process each message. Funnily enough I found this rant today.

Edit: Thought of another annoying case, what if the eyeball requests a normal websocket without compression, and their worker is proxying the websocket connection? Then the eyeball <-> worker is uncompressed, and the worker <-> origin server is compressed (assuming server accepts compression). This would prevent us from doing an optimized pump of the raw bytes.

[jasnell]

The scariest thing to me is that we'd be going from a world where we have no compression to a world where (potentially) all websocket communication is compressed.

Perhaps a compatibility flag is warranted here?

[bretthoerner]

Thought of another annoying case, what if the eyeball requests a normal websocket without compression, and their worker is proxying the websocket connection? Then the eyeball <-> worker is uncompressed, and the worker <-> origin server is compressed (assuming server accepts compression). This would prevent us from doing an optimized pump of the raw bytes.

Oh, interesting. Do many UA's actually not request compression? Anyway, I wonder if fetch subrequests should default to what the eyeball requested?

[MellowYarker]

The scariest thing to me is that we'd be going from a world where we have no compression to a world where (potentially) all websocket communication is compressed.

Perhaps a compatibility flag is warranted here?

I think we won't need it on new WebSocket() since we already have a flag blocking compression, but if we decide we also want fetch() to default to compression, then a flag is probably a good idea there.

I'm not as concerned about new WebSocket() because

• It should be expected that this compresses by default,
• We only made that API functional a couple months ago and I don't think we updated public docs to indicate that it works now, so it feels reasonable to assume most websockets still use fetch().
• Since compression is already behind a compat flag, the change won't be in affect until we remove the other flag, but I currently can't test compression with new WebSocket() and putting it behind another flag means we have to bump the validator and re-release EWC. Could test sooner if we didn't require an additional flag for this particular API.

Feel free to push back here!

---

Oh, interesting. Do many UA's actually not request compression?

I don't think we collect metrics on that, adding a metric to count the number of inbound/outbound permessage-deflate requests is actually on my todo list. It's probably safe to assume any browser client is requesting it, but if there are native apps or embedded devices that are using a websocket library then it's possible their library doesn't support compression at all/doesn't send the header by default.

Anyway, I wonder if fetch subrequests should default to what the eyeball requested?

Maybe a dumb question but don't we do this already here? Unless jsRequest doesn't refer to the original inbound request to the worker :thinking:. I know there's a difference between fetch(url) and fetch(request, otherStuff), but I'm not really familiar with the fetch implementation. I vaguely remember being unable to read the header values (as in the actual value, not the header name) from the IoContext, in which case it's not super obvious to me how we could get the original request's header value.

[MellowYarker]

Since compression is still behind a compat flag, could we merge this PR as is? If the (existing) compat flag isn't provided, we strip the header right after anyways.

We can circle back to fetch's behavior some time before we remove the compat flag, but for now it seems clear that new WebSocket() should include this header. It'd allow me to test that new WebSocket() is working correctly, and it looks like there's an ongoing effort to fuzz websocket messages, so enabling this by default would get us some extra compression test coverage.