feat: sketch out utls http.RoundTripper #74
Conversation
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
nitpick: I'd prefer a more telling naming scheme for those transports, something like
h1Cleartext
h1Secure
h2Secure
|
|
||
| // init checks whether the user wants verbose logs. | ||
| func init() { | ||
| if os.Getenv("UHTTP_VERBOSE") == "1" { |
There was a problem hiding this comment.
Please add a function that will let users enable verbose logging without having to set environment variables (and mention it in the readme).
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
After a brief look I didn't understand the purpose of onlyDial.
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
Please lower the minimum version as low as possible.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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}. |
There was a problem hiding this comment.
Let's not indicate success by returning errors. Seems like this will need some refactoring.
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
Don't we need to lock earlier?
There was a problem hiding this comment.
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.)
|
Thanks for the quick first round of review! :-) |
|
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 |
|
@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? |
|
@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. |
We have experimental OONI code that is similar to an http.RoundTripper for
utlsas 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.)