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

feat: sketch out utls http.RoundTripper #74

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bassosimone
Copy link

@bassosimone bassosimone commented Mar 23, 2021

We have experimental OONI code that is similar to an http.RoundTripper for utls as described in #16. So, I choose to spend some time to make one. If you like the concept, then I can also write and add unit tests to this PR. (The same code is also at https://github.com/bassosimone/utlstransport where it can be tried out with a test client.)

Copy link
Member

@sergeyfrolov sergeyfrolov left a comment

Choose a reason for hiding this comment

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

Hi Simone. I took a brief look, and I do like the concept -- many thanks for the contribution! I didn't quite figure out how caching works yet, but I left some comments there, will take another look later.
Please don't forget to add a section in README to tell people how to use this.

https uhttpCloseableTransport

// h2 is like https but for the "h2" ALPN.
h2 uhttpCloseableTransport
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I'd prefer a more telling naming scheme for those transports, something like
h1Cleartext
h1Secure
h2Secure

Copy link
Author

Choose a reason for hiding this comment

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

Sure!


// init checks whether the user wants verbose logs.
func init() {
if os.Getenv("UHTTP_VERBOSE") == "1" {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a function that will let users enable verbose logging without having to set environment variables (and mention it in the readme).

Copy link
Author

Choose a reason for hiding this comment

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

Sure! (The rationale of this design with the environment variable was to do what the standard library does in terms of logging: they check the GODEBUG environment variable to determine whether to emit debug events.)

// connections. It will populate connCache and
// hostCache. We will initialize this field during
// the first invocation of RoundTrip.
onlyDial uhttpCloseableTransport
Copy link
Member

Choose a reason for hiding this comment

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

After a brief look I didn't understand the purpose of onlyDial.

Copy link
Author

Choose a reason for hiding this comment

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

The http/1.1 and the h2 transports are configured to reject any configuration attempt with an error saying that there are no cached connections (errUHTTPNoCachedConn). The onlyDial transport is only used for dialing TCP/UTLS connections. In turn, onlyDial is configured to create connections, store them into a cache as a side effect, and then return an error indicating whether to use http/1.1 or h2 based on the the ALPN value. Maybe I can explain in much more detail this fact in the documentation of onlyDial?

@@ -0,0 +1,8 @@
module github.com/refraction-networking/utls

go 1.16
Copy link
Member

Choose a reason for hiding this comment

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

Please lower the minimum version as low as possible.

Copy link
Author

Choose a reason for hiding this comment

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

Sure! I suppose the minimum required version is Go 1.14 where DialTLSContext was added to http.Transport. (I am also using log.Default, which is Go >= 1.16, but that functionality is definitely not fundamental to this patch.)

}
// Step 3: dispatch HTTPS requests to the proper transport
child := txp.hostCacheGetOrDefault(req.URL)
const maxRetries = 4
Copy link
Member

Choose a reason for hiding this comment

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

We should allow users to change maxRetries, perhaps as a field in UHTTPTransport

// settings configured inside UHTTPTransport. This function
// updates hostCache and saves the connection into connCache,
// on success. Note that success is indicated by returning
// one of errUHTTPUse{H2,HTTPS}.
Copy link
Member

Choose a reason for hiding this comment

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

Let's not indicate success by returning errors. Seems like this will need some refactoring.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Side effects and errors as sentinels are bad. Thank you for your comment, this is really what code review is about! I suppose there is here a good opportunity for simplifying the codebase by getting rid of onlyDial and running the dialUTLSContext right inside RoundTrip. By running the diff on memory, this seems possible to do and it also seems to yield a simpler algorithm inside of RoundTrip, which is also good.

txp.h2.CloseIdleConnections()
}
defer txp.mu.Unlock()
txp.mu.Lock()
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to lock earlier?

Copy link
Author

Choose a reason for hiding this comment

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

If goroutine A calls RoundTrip and goroutine B calls CloseIdleConnections concurrently, we have a data race because the write on, say, txp.h2 is not guaranteed to happen before the read that we do here.

(I would still like the CloseIdleConnections to go unlocked, because it does not need to synchronize with any other synchronized access performed by this structure, but I should refactor the code to avoid this race.)

@bassosimone
Copy link
Author

Thanks for the quick first round of review! :-)

@bassosimone
Copy link
Author

Okay, @sergeyfrolov, I've gone through the comments, and it seems to me the bottom line here is that there is an opportunity for refactoring to avoid side effects, the need to reorganize locking in CloseIdleConnections and a bunch of nitpicks. Will do that!

@max-b
Copy link
Contributor

max-b commented Aug 27, 2021

@bassosimone any luck moving this forwards? Do you have an idea of whether you'll have the time to do this or whether it might be worthwhile for someone (maybe me???) to pick it up?

@bassosimone
Copy link
Author

@max-b thanks for asking! I think I am unlikely to have time anytime soon because with OONI we choose in the end to use another approach to solve this problem (https://github.com/ooni/oohttp). So, if you or anyone else wants to move this patch forward, please do that! Please, feel free to ping me if you need any clarification regarding this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants