This repository has been archived by the owner on Mar 6, 2020. It is now read-only.
netx: move beginning and handler at toplevel #39
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We'll do less TLS handshakes. It is true that with TLSv1.3 the handshake is faster. But it is also nicer to servers to not open too many connections at a time. So, this seems quite right.
Merge dialerbase into dialerapi since dialerbase was basically just a small dialerapi implementation detail. Ensure that the input parameters to the constructor and the dependencies of dialerbase are not weird stuff. Try to limit exposure of implementation details or weird types as much as possible. Start thinking about deprecating some code parts that are not much used and tricky.
The godns was bringing with it a significant baggage of complexity however it did not work on all platforms. As such, it has become an obvious target for pruning. It is in the way of more refactoring and it's not adding value. While there, also fix a bunch of small issues that I've met.
So, the current idea is to use the same pattern used by the dnstransports, and have different clients one can compose. The most obvious, initial configuration is the one where one is decomposing doing the real business and emitting events.
This starts to show the direction where I'm heading: 1. use interfaces more often 2. compose behaviours by composing interfaces 3. use context rather than global handlers This is exemplified by delegating emitting the Resolve event to a wrapper of a generic dnsclient, which may even be system. In a follow-up PR, I'll add extra code for checking whether an IP address is legit and possibly retry if not. While there, move the basic interfaces that qualify DNS into model because I constantly clash with them being in dsnx when refactoring, since their presence and their low level usage basically makes the dnsx package useless, as I can't add functions there.
This is already important now, since the context carries a cancellation signal, but will be even more important if/when we manage to propagate measurement context using it.
These smaller pieces implement the interface pattern where you can wrap a basic piece with another implementing the same interface. I want to potentially retry connecting with some kind of proxy and see whether it's possible to circumvent blocking. I want to potentially retry the TLS handshake by changing the case of the certificate, by maybe splitting it into multiple segments. What's more, now all these new pieces use a context to carry around information rather using a handler passed to constructor. We cannot still really take advantage of this, because we're still stuck with a dialerapi.Dialer that configures its own policy inside of its constructor, but we'll make that more flexible soon.
The final barrier for using context for propagating transaction info is the usage of DialTLS inside the http transport. By switching to using DialContext instead, and by delegating to the net/http transport the TLS handshake, we can propagate the context everywhere, so we can always put measurements in context. While working on that, I realised that the split between a TLS handshake start and a TLS handshake done event was reasonable since it allowed us to put I/O events between these two events in the right context (i.e. TLS handshake I/O) as well. In the same vein, I should probably split the Resolve event into two separate events, to put resolve operations in context.
All the netx code now does not depend on a preconfigured beginning and handler, except toplevel code. This is very helpful because it allows us to specify different contexts depending on circumstances. Yet, the public API has not changed because there is still a notion of the binding of a dialer with a handler. What I like about this change is that it allows us to evolve our code potentially in two directions: 1. we keep a single handler and we provide correct information concerning the current TransactionID 2. we switch to a rooted handlers model where the handler is passed as part of the context Now, internals in netx already follow model #2, while the toplevel API is still using model #1. The part that needs refactoring now is httpx.
bassosimone
added this to In progress
in DO NOT USE (was: OONI Nettests and Engine)
via automation
|
I ended up implementing this in a slightly different way and merging it into master |
DO NOT USE (was: OONI Nettests and Engine)
automation
moved this from In progress
to Done
Oct 27, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All the netx code now does not depend on a preconfigured
beginning and handler, except toplevel code.
This is very helpful because it allows us to specify different
contexts depending on circumstances.
Yet, the public API has not changed because there is still
a notion of the binding of a dialer with a handler.
What I like about this change is that it allows us to evolve
our code potentially in two directions:
we keep a single handler and we provide correct information
concerning the current TransactionID
we switch to a rooted handlers model where the handler is
passed as part of the context
Now, internals in netx already follow model
#2, while the toplevelAPI is still using model
#1.The part that needs refactoring now is httpx.