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.

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

Re-enable http/2 #95

Closed
wants to merge 2 commits into from
Closed

Re-enable http/2 #95

wants to merge 2 commits into from

Conversation

willscott
Copy link
Collaborator

By setting a custom TLSClientConfig we remove the default of having a NextProtos set and need to add it back

@willscott willscott requested review from lidel and hacdias April 13, 2023 21:05
By setting a custom TLSClientConfig we remove the default of having a `NextProtos` set and need to add it back
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Thanks! I suspect it is not that easy, we miss something:

failed to resolve /ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi: Bad Gateway: http request failed: Get "https://38.93.247.224/ipfs/bafybeigdyrzt5sfp7udm7hu76uh7y26nf3efuylqabf3oclgtqy55fbzdi?format=raw": http2: no cached connection was available

Unless you get to this first, I can look into this after IPFS Thing.

Quick thoughts:

  • good opportunity to clean up and move generic client things to http_client.go so HTTP2-enabled client can be reused by blockstore_proxy.go and routing.go, ensuring we benefit from HTTP/2 everywhere
    • caveat: non-HTTPS endpoints like PROXY_GATEWAY_URL=http://127.0.0.1:8080 need to work, this is what we use in conformance test on CI, and using the same HTTP client there will be a good idea
      • Maybe http2.Transport{AllowHTTP: true will do the trick?

@willscott
Copy link
Collaborator Author

explanation potentially around context of the 'no cached connection available' at golang/go#16582

@willscott
Copy link
Collaborator Author

@lidel - i think we want ahttp2.Transport at the basis instead of a an http.Transport. That means a custom DialTLSContext method to make use of the cached DNS dialer, unfortunately.
See if this basic structure seems plausible.

@willscott
Copy link
Collaborator Author

It looks like we could potentially just set ForceAttemptHTTP2 on the standard http transport. I'll try simplifying to that
https://go.googlesource.com/go/+/master/src/net/http/transport.go#288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants