Conversation
|
Review requested:
|
test/common/websocket-server.js
Outdated
| this.customHandleUpgradeHeaders.map((header) => { | ||
| const index = header.indexOf(':'); | ||
| return [header.slice(0, index).trim(), header.slice(index + 1).trim()]; | ||
| }) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #62570 +/- ##
==========================================
- Coverage 89.71% 89.71% -0.01%
==========================================
Files 695 695
Lines 214154 214441 +287
Branches 41009 41057 +48
==========================================
+ Hits 192132 192383 +251
- Misses 14075 14113 +38
+ Partials 7947 7945 -2 🚀 New features to boost your workflow:
|
| cert: fixtures.readKey('agent1-cert.pem'), | ||
| allowHTTP1: true, | ||
| settings: { | ||
| enableConnectProtocol: true, |
There was a problem hiding this comment.
This invalidates this test and needs changing. The test is specifically checking that we do support HTTP/1 upgrades in HTTP/2 servers with allowHttp1. With this change this test does websockets over HTTP/2 CONNECT instead and skips HTTP/1 entirely. We do need to cover the HTTP/1 upgrade support somehow since it's widely used and it did break recently.
I'm unclear about how the Undici H2 default changes will handle this if you just disable the setting... In theory, the client should see from the settings frame that CONNECT isn't supported, and so fall back websockets over HTTP/1 automatically. We should probably add an assertion in this test to confirm the resulting connection is actually HTTP/1, now that this websocket server supports both.
I guess you added this because otherwise this test fails though, which is a major issue. If this change is actually required, then this whole bump is at least a semver-major change and probably not shippable at all, since it will break lots of websocket connections to servers that only support websockets over HTTP/1. There's a lot of these, I suspect a majority of servers. E.g. AWS LBs apparently only support websockets over HTTP/1, same for NGINX, etc.
We can default to HTTP/2 for websockets, but we have to be able to transparently downgrade when it's not available, so this test needs to pass without changes.
There was a problem hiding this comment.
Ah I misunderstood the test. I'll fix it in another way (forcing http/1.1 on the other side).
There might be an actual regression.
| assert.strictEqual(stderr.trim(), ''); | ||
| assert.match(stdout, /Status Code: 200/); | ||
| assert.match(stdout, /Hello world/); | ||
| assert.strictEqual(code, 0); |
There was a problem hiding this comment.
Why does this change? Is that expected? It doesn't look like this uses Undici at all... AFAICT runProxiedRequest sends the request with http or https, and doesn't use Undici or fetch anywhere. It's a debatable behaviour change in itself anyway - going from rejecting invalid config to silently ignoring it.
No description provided.