Proposals for improving netx API and implementation #420
Conversation
bassosimone
changed the title
bassosimone
changed the title
After a few days spent mulling over #125 and #359, I reached the conclusion that we most likely want to heavily refactor netx. This is even more true in light of the introduction of DialTLSContext instead of Go 1.14, which allows us to more cleanly separate the place where we allow tapping into events from the place in which we collect such events. This diff distills some code I've been working on for the past few days into a single file where we make all kind of tapping/wrapping possible and easy.
bassosimone
changed the title
| return nil, err | ||
| } | ||
| return &connWrapper{Conn: conn, Counter: c}, err | ||
| } |
There was a problem hiding this comment.
The specific problem of this implementation is that the byte counter is also a dialer, therefore we cannot separate the byte counting and the dialing. However, we sometimes want to account for session and sometimes for both session and experiment. Therefore, this implementation here is not actually meeting our objectives.
| // 3. otherwise, it returns the OONI failure string | ||
| // | ||
| // See the netx/DESIGN.md for more info. | ||
| func Failure(err error) string { |
There was a problem hiding this comment.
Failure and Operation are quite useful and there should probably be an issue about them.
| type Event struct { | ||
| Op string | ||
| Time time.Time | ||
| } |
There was a problem hiding this comment.
It is probably more convenient to be able to wrap every Dialer independently of, say, a Resolver, because this gives us more flexibility. Also, a best practice of Go is to keep interfaces small, so...
| Handshake(ctx context.Context, conn net.Conn, config *tls.Config) ( | ||
| net.Conn, tls.ConnectionState, error) | ||
| RoundTrip(req *http.Request) (*http.Response, error) | ||
| } |
There was a problem hiding this comment.
As said earlier it would be more flexible to have several smaller interfaces.
| func ContextOperations(ctx context.Context) Operations { | ||
| operations, _ := ctx.Value(operationsKey{}).(Operations) | ||
| return operations | ||
| } |
There was a problem hiding this comment.
This design allows us to change basically everything with every request using a context. This is nice but at the same time it becomes very confusing to understand what is happening when you are reading the code. It seems that we should not pursue this strategy and rather see to use the context as sparingly as possible.
|
There is too much magic when we're using the context to possibly hijack every operation. It seems better to use the context much less frequently as we do in #421 |
This pull request contains a bunch of patches aimed at improving the netx API and implementation. It is meant as a companion of #359, where we specify how such changes should look like. It has been originally triggered by work on #125 and on #404, where I learned a lot about ways in which we can evolve netx. At the moment of opening this PR, the plan is to keep this as a Draft PR for documentation purposes, and cherry-pick specific patches if/when needed.