Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
mafredri wants to merge1 commit intomaster
base:master
Choose a base branch
Loading
frommafredri/http2

Conversation

mafredri
Copy link
Member

@mafredrimafredri commentedSep 29, 2025•
edited
Loading

This change adds experimental support for WebSockets over HTTP/2 (RFC 8441).

Implementation notes:

  • To avoid breaking any flows, HTTP/2 functionality is currently opt-in on both server and client
  • The HTTP/2 tests are currently kept ininternal/thirdparty/http2 (they require importinggolang.org/x/net/http2, seenet/http: move HTTP/2 into std golang/go#67810)
  • HTTP/2 extended CONNECT must currently be enabled viaGODEBUG, seenet/http: add support for SETTINGS_ENABLE_CONNECT_PROTOCOL golang/go#53208
  • Explicit selection of HTTP/1 or HTTP/2 is required duringDial because we cannot test the underlying transport for support (for instance,http.Transport does not allow setting the HTTP/2 pseudo-header:protocol)
  • ToDial with HTTP/2 support, ahttp2.Transport must be provided viaDialOptions.Client (since we do not importgolang.org/x/net/http2)
  • There are a few changes and cleanups unrelated to this feature, if needed they can be broken out into a separate branch

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.

ammario and code-asher reacted with heart emoji
@mafredrimafredriforce-pushed themafredri/http2 branch 11 times, most recently from469125f to9470379CompareSeptember 29, 2025 18:59
@mafredrimafredri self-assigned thisSep 29, 2025
@mafredrimafredri marked this pull request as ready for reviewOctober 1, 2025 13:09
@mtojekmtojek self-requested a reviewOctober 1, 2025 14:06
-uses:actions/setup-go@v5
with:
go-version-file:./go.mod
# HACK(mafredri): The exampels and thirdparty library require Go 1.24

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

Suggested change
# HACK(mafredri): Theexampels and thirdparty library require Go 1.24
# HACK(mafredri): Theexamples and thirdparty library require Go 1.24

Copy link
Member

@code-ashercode-asher left a 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.

mafredri reacted with heart emoji

```console
#Server.
$cd examples/http2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should it becd internal/examples/http2 or is the readme from the context of being ininternal already? (And same for the othercds below).

Copy link
MemberAuthor

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.

code-asher reacted with rocket emoji
funcvisibleAddr(addrstring)string {
// If binding to all interfaces with ":port", display "127.0.0.1:port".
ifstrings.HasPrefix(addr,":") {
return"127.0.0.1"+addr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would0.0.0.0:port be better to indicate binding on all interfaces? To me 127.0.0.1 implies loopback only.

mafredri reacted with thumbs up emoji
}

funcverifyServerResponseH1(opts*DialOptions,copts*compressionOptions,secWebSocketKeystring,resp*http.Response) (*compressionOptions,error) {
ifresp.StatusCode!=http.StatusSwitchingProtocols {
Copy link
Member

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 checkresp.ProtoMajor here like the other verify functions?

Copy link
MemberAuthor

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.

code-asher reacted with thumbs up emoji
Copy link
Member

@mtojekmtojek left a 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?

mafredri reacted with heart emoji
with:
name:coverage.html
path:./ci/out/coverage.html
bench-dev:
Copy link
Member

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 :)

Copy link
MemberAuthor

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.
typeProtocolint
Copy link
Member

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.

Copy link
MemberAuthor

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 didHTTPProtocol 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.

mtojek reacted with thumbs up emoji

// ProtocolHTTP1 selects HTTP/1.1 GET+Upgrade for the WebSocket handshake.
// This is the default (zero value).
ProtocolHTTP1
Copy link
Member

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 toHTTP3.99 :)

Copy link
MemberAuthor

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"
gotest --bench=. --timeout=1h -cover -covermode=atomic -coverpkg="$coverpkgsjoined" -test.gocoverdir="$coverbase/race=0""$@" ./...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

1h?

Copy link
MemberAuthor

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"
Copy link
Member

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?

mafredri reacted with thumbs up emoji
case"OK","INFORMATIONAL":
default:
t.Errorf("bad closebehaviour")
t.Errorf("bad closebehavior")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

}

selfSigned:=""
if*certFile==""&&*keyFile=="" {
Copy link
Member

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

Copy link
MemberAuthor

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 👍🏻

mtojek reacted with thumbs up emoji
});ok {
ginWriter.WriteHeaderNow()
switchopts.Protocol {
caseProtocolHTTP2:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should we supporthttp.UnencryptedHTTP2?

Copy link
MemberAuthor

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 👍🏻

@code-asher
Copy link
Member

code-asher commentedOct 3, 2025•
edited
Loading

Have you tried to establish a connection using other tools? JS possibly?

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 (needsws).

consthttp2=require("http2")const{ Receiver, Sender}=require("ws")constclient=http2.connect("http://127.0.0.1:8080")constreq=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)})constreceiver=newReceiver()receiver.on("message",(data)=>{console.log("got message:",data.toString())})req.on("data",(chunk)=>{receiver.write(chunk)})setInterval(()=>{constsender=newSender(req)sender.send("hey",{binary:false,mask:true,fin:true})},1000)

sample output:

status: 200got message: heygot message: heygot message: hey

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@zedkippzedkippzedkipp left review comments

@mtojekmtojekmtojek approved these changes

@code-ashercode-ashercode-asher approved these changes

Assignees

@mafredrimafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WebSockets over HTTP/2
4 participants
@mafredri@code-asher@zedkipp@mtojek
[8]ページ先頭
©2009-2025 Movatter.jp