Skip to content
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.

Sign up for GitHub

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

Jump to bottom

server: Use http.Transport for HTTP client #2110

Merged
merged 3 commits into from
Nov 22, 2021
Merged

server: Use http.Transport for HTTP client #2110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dffe6cc
server: Use http.Transport for HTTP client
yondonfu Nov 18, 2021
dd00c91
server: Enable ForceAttemptHTTP2 on HTTP client
yondonfu Nov 19, 2021
cd334a0
CHANGELOG_PENDING: Transparently support HTTP/2
yondonfu Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG_PENDING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
- \#2022 Randomize selection of orchestrators in untrusted pool at a random frequency (@yondonfu)
- \#2100 Check verified session first while choosing the result from multiple untrusted sessions (@leszko)
- \#2103 Suspend sessions that did not pass p-hash verification (@leszko)
- \#2110 Transparently support HTTP/2 for segment requests while allowing HTTP/1 via GODEBUG runtime flags (@yondonfu)

#### Orchestrator

Expand Down
21 changes: 14 additions & 7 deletions server/segment_rpc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import (
"github.com/livepeer/go-livepeer/net"
"github.com/livepeer/lpms/ffmpeg"
"github.com/livepeer/lpms/stream"
"golang.org/x/net/http2"

ethcommon "github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
Expand All @@ -44,16 +43,24 @@ var errProfile = errors.New("unrecognized encoder profile")
var errDuration = errors.New("invalid duration")
var errCapCompat = errors.New("incompatible capabilities")

var dialTimeout = 2 * time.Second

var tlsConfig = &tls.Config{InsecureSkipVerify: true}
var httpClient = &http.Client{
Transport: &http2.Transport{
Transport: &http.Transport{
TLSClientConfig: tlsConfig,
DialTLS: func(network, addr string, cfg *tls.Config) (gonet.Conn, error) {
netDialer := &gonet.Dialer{
Timeout: 2 * time.Second,
}
return tls.DialWithDialer(netDialer, network, addr, cfg)
DialTLSContext: func(ctx context.Context, network, addr string) (gonet.Conn, error) {
victorges marked this conversation as resolved.
Show resolved Hide resolved
cctx, cancel := context.WithTimeout(ctx, dialTimeout)
defer cancel()

tlsDialer := &tls.Dialer{Config: tlsConfig}
Copy link
Member

@victorges victorges Nov 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Should this be shared among multiple connections (declared on the same lvl as tlsConfig and httpClient or is it ok/advised to instantiate a new one for every dial?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure, but I haven't seen any information recommending re-using a single dialer or indicating that instantiating a new dialer for each dial could result in inefficiencies. One data point here is that the code for tls.Dial actually instantiates a new net.Dialer (not tls.Dialer, but I imagine both types would have the same considerations) under the hood whenever it is called so that makes me feel like instantiating a new tls.Dialer in our case is probably fine.

return tlsDialer.DialContext(cctx, network, addr)
},
// Required for the transport to try to upgrade to HTTP/2 if TLSClientConfig is non-nil or
// if custom dialers (i.e. via DialTLSContext) are used. This allows us to by default
// transparently support HTTP/2 while maintaining the flexibility to use HTTP/1 by running
// with GODEBUG=http2client=0
ForceAttemptHTTP2: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

According to the http.Transport docs and the http package code, ForceAttemptHTTP2: true is needed for the transport to try to upgrade to HTTP/2 if TLSClientConfig is non-nil or if custom dialers (i.e. DialTLSContext) are used. By including this, we can default to transparently supporting HTTP/2 while maintaining the flexibility to use HTTP/1 by running with GODEBUG=http2client=0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You can add a comment into the code for the explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in ca94081

},
// Don't set a timeout here; pass a context to the request
}
Expand Down