-
Notifications
You must be signed in to change notification settings - Fork 329
feat: add experimental http2 support #539
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
base: master
Are you sure you want to change the base?
Conversation
469125f
to
9470379
Compare
0616f05
to
047d1c0
Compare
- uses: actions/setup-go@v5 | ||
with: | ||
go-version-file: ./go.mod | ||
# HACK(mafredri): The exampels and thirdparty library require Go 1.24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo here and on line 33
# HACK(mafredri): The exampels and thirdparty library require Go 1.24 | |
# HACK(mafredri): The examples and thirdparty library require Go 1.24 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice 🤌 I messed around with the example as well and it all looks good.
|
||
```console | ||
# Server. | ||
$ cd examples/http2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be cd internal/examples/http2
or is the readme from the context of being in internal
already? (And same for the other cd
s below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually yeah, good catch, we'll need to update the other examples too but I can do that in a separate PR.
func visibleAddr(addr string) string { | ||
// If binding to all interfaces with ":port", display "127.0.0.1:port". | ||
if strings.HasPrefix(addr, ":") { | ||
return "127.0.0.1" + addr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would 0.0.0.0:port
be better to indicate binding on all interfaces? To me 127.0.0.1 implies loopback only.
} | ||
|
||
func verifyServerResponseH1(opts *DialOptions, copts *compressionOptions, secWebSocketKey string, resp *http.Response) (*compressionOptions, error) { | ||
if resp.StatusCode != http.StatusSwitchingProtocols { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to check resp.ProtoMajor
here like the other verify functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it wouldn't hurt 🤔, I'll do that in a separate PR though as it's touching the H1 code, want to avoid changing it too much in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing skills 👍 👍
Have you tried to establish a connection using other tools? JS possibly?
with: | ||
name: coverage.html | ||
path: ./ci/out/coverage.html | ||
bench-dev: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this just maintenance or related to this PR? If the first one, perhaps split it... just in case we have to revert this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintenance, I'll break the unrelated stuff into a separate PR 👍🏻
// an http2.Transport (from golang.org/x/net/http2). | ||
// | ||
// Experimental: This type is experimental and may change in the future. | ||
type Protocol int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/Protocol/HTTPProtocolVersion? I suppose there won't be different protocols than HTTP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I was a bit on the fence about this myself, I almost did HTTPProtocol
but it's a mouth/eyefull with the PP. 😅
On Dial we do have HTTPClient, HTTPHeader, so it's not too much of a stretch to do HTTPProtocol.
|
||
// ProtocolHTTP1 selects HTTP/1.1 GET+Upgrade for the WebSocket handshake. | ||
// This is the default (zero value). | ||
ProtocolHTTP1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This model will work until one day we will need to switch to HTTP3.99
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProtocolHTTP3_99
😂
coverpkgsjoined=$(IFS=,; echo "${coverpkgs[*]}") | ||
|
||
echo "++++ Running main tests" | ||
go test --bench=. --timeout=1h -cover -covermode=atomic -coverpkg="$coverpkgsjoined" -test.gocoverdir="$coverbase/race=0" "$@" ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1h?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a typical timeout in this project all around. I'll keep it like that but we can follow-up with a cleanup regarding that.
mkdir -p ./ci/out/static | ||
cp ./ci/out/coverage.html ./ci/out/static/coverage.html | ||
percent=$(go tool cover -func ./ci/out/profile.txt | tail -n1 | awk '{print $3}' | tr -d '%') | ||
wget -O ./ci/out/static/coverage.svg "https://img.shields.io/badge/coverage-${percent}%25-success" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: perhaps extract it to a different PR?
case "OK", "INFORMATIONAL": | ||
default: | ||
t.Errorf("bad close behaviour") | ||
t.Errorf("bad close behavior") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
} | ||
|
||
selfSigned := "" | ||
if *certFile == "" && *keyFile == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct me if I'm wrong, a certificate is mandatory, right? if not, I would just simplify the example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the auto-generated self-signed cert to make testing the example easier, but we can enforce cert/key, I was on the fence about adding cert generation anyway 👍🏻
}); ok { | ||
ginWriter.WriteHeaderNow() | ||
switch opts.Protocol { | ||
case ProtocolHTTP2: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we support http.UnencryptedHTTP2
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know about this, I'll need to investigate. Thanks for the heads up 👍🏻
While I was testing I experimented with connecting to the server example from a JS (Node) client. Here is what I used for anyone interested (needs const http2 = require("http2")
const { Receiver, Sender } = require("ws")
const client = http2.connect("http://127.0.0.1:8080")
const req = client.request({
":method": "CONNECT",
":protocol": "websocket",
":scheme": "http",
":path": "/",
":authority": "127.0.0.1",
"Sec-WebSocket-Key": "AQIDBAUGBwgJCgsMDQ4PEC==",
"Sec-WebSocket-Version": "13",
})
req.on("response", (headers) => {
console.log("status:", headers[":status"])
})
req.on("error", (err) => {
console.log("error:", err)
})
const receiver = new Receiver()
receiver.on("message", (data) => {
console.log("got message:", data.toString())
})
req.on("data", (chunk) => {
receiver.write(chunk)
})
setInterval(() => {
const sender = new Sender(req)
sender.send("hey", { binary: false, mask: true, fin: true })
}, 1000) sample output:
|
This change adds experimental support for WebSockets over HTTP/2 (RFC 8441).
Implementation notes:
internal/thirdparty/http2
(they require importinggolang.org/x/net/http2
, see net/http: move HTTP/2 into std golang/go#67810)GODEBUG
, see net/http: add support for SETTINGS_ENABLE_CONNECT_PROTOCOL golang/go#53208Dial
because we cannot test the underlying transport for support (for instance,http.Transport
does not allow setting the HTTP/2 pseudo-header:protocol
)Dial
with HTTP/2 support, ahttp2.Transport
must be provided viaDialOptions.Client
(since we do not importgolang.org/x/net/http2
)Closes #4
This change does not yet include benchmark tests for HTTP/2, those will be added at a later date as a follow-up.