-
Notifications
You must be signed in to change notification settings - Fork 54
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
refactor(enginenetx): rename HTTPTransport to Network (#1293)
The HTTPTransport model only offers a CloseIdleConnections callback, which semantics is obviously that of closing idle connections (unsurprisingly). However, the struct I am slowly building inside the enginenetx package soon will need to write back statistics to disk using a key-value store. I don't think we should overload the CloseIdleConnections semantics to do this job, since the resulting code would be quite surprising. Therefore, I have decided to rename HTTPTransport to Network and make it represent all the network abstractions required by the OONI engine. In time, I will move extra functionality in there. For now, let us be happy that we can easily define a Close method (currently empty) for this type having the usual io.Closer semantics, i.e., that any resource opened by the type itself is released when calling this method. This diff does the following: - it renames the files, the type, and the tests; - it introduces a Close method that closes the Network's underlying transport's idle connections; - it adapts users of this code to use the new semantics; - it introduces an model.KeyValueStore argument for the Network constructor which we're going to use soon to persist statistics; - updates `CONTRIBUTING.md` to say how we do internal testing when the main tests body is external. Part of ooni/probe#2531
- Loading branch information
1 parent
1e42525
commit e27eead
Showing
8 changed files
with
143 additions
and
72 deletions.
There are no files selected for viewing
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
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
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
This file was deleted.
Oops, something went wrong.
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package enginenetx | ||
|
|
||
| import ( | ||
| "net/http" | ||
| "net/http/cookiejar" | ||
| "net/url" | ||
|
|
||
| "github.com/ooni/probe-cli/v3/internal/bytecounter" | ||
| "github.com/ooni/probe-cli/v3/internal/model" | ||
| "github.com/ooni/probe-cli/v3/internal/netxlite" | ||
| "github.com/ooni/probe-cli/v3/internal/runtimex" | ||
| "golang.org/x/net/publicsuffix" | ||
| ) | ||
|
|
||
| // Network is the network abstraction used by the OONI engine. | ||
| type Network struct { | ||
| txp model.HTTPTransport | ||
| } | ||
|
|
||
| // HTTPTransport returns the [model.HTTPTransport] that the engine should use. | ||
| func (n *Network) HTTPTransport() model.HTTPTransport { | ||
| return n.txp | ||
| } | ||
|
|
||
| // NewHTTPClient is a convenience function for building a [model.HTTPClient] using | ||
| // the underlying [model.HTTPTransport] and the correct cookies configuration. | ||
| func (n *Network) NewHTTPClient() *http.Client { | ||
| // Note: cookiejar.New cannot fail, so we're using runtimex.Try1 here | ||
| return &http.Client{ | ||
| Transport: n.txp, | ||
| Jar: runtimex.Try1(cookiejar.New(&cookiejar.Options{ | ||
| PublicSuffixList: publicsuffix.List, | ||
| })), | ||
| } | ||
| } | ||
|
|
||
| // Close ensures that we close idle connections and persist statistics. | ||
| func (n *Network) Close() error { | ||
| // TODO(bassosimone): do we want to introduce "once" semantics in this method? | ||
|
|
||
| // make sure we close the transport's idle connections | ||
| n.txp.CloseIdleConnections() | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // NewNetwork creates a new [*Network] for the engine. This network MUST NOT be | ||
| // used for measuring because it implements engine-specific policies. | ||
| // | ||
| // You MUST call the Close method when done using the network. This method ensures | ||
| // that (i) we close idle connections and (ii) persist statistics. | ||
| // | ||
| // Arguments: | ||
| // | ||
| // - counter is the [*bytecounter.Counter] to use. | ||
| // | ||
| // - kvStore is a [model.KeyValueStore] for persisting stats; | ||
| // | ||
| // - logger is the [model.Logger] to use; | ||
| // | ||
| // - proxyURL is the OPTIONAL proxy URL; | ||
| // | ||
| // - resolver is the [model.Resolver] to use. | ||
| // | ||
| // The presence of the proxyURL will cause this function to possibly build a | ||
| // network with different behavior with respect to circumvention. If there is | ||
| // an upstream proxy we're going to trust it is doing circumvention for us. | ||
| func NewNetwork( | ||
| counter *bytecounter.Counter, | ||
| kvStore model.KeyValueStore, | ||
| logger model.Logger, | ||
| proxyURL *url.URL, | ||
| resolver model.Resolver, | ||
| ) *Network { | ||
| dialer := netxlite.NewDialerWithResolver(logger, resolver) | ||
| handshaker := netxlite.NewTLSHandshakerStdlib(logger) | ||
| tlsDialer := netxlite.NewTLSDialer(dialer, handshaker) | ||
| txp := netxlite.NewHTTPTransportWithOptions( | ||
| logger, dialer, tlsDialer, | ||
| netxlite.HTTPTransportOptionDisableCompression(false), | ||
| netxlite.HTTPTransportOptionProxyURL(proxyURL), // nil implies "no proxy" | ||
| ) | ||
| txp = bytecounter.WrapHTTPTransport(txp, counter) | ||
| return &Network{txp} | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package enginenetx | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/ooni/probe-cli/v3/internal/mocks" | ||
| ) | ||
|
|
||
| func TestNetworkUnit(t *testing.T) { | ||
| t.Run("HTTPTransport returns the correct transport", func(t *testing.T) { | ||
| expected := &mocks.HTTPTransport{} | ||
| netx := &Network{txp: expected} | ||
| if netx.HTTPTransport() != expected { | ||
| t.Fatal("not the transport we expected") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("Close calls the transport's CloseIdleConnections method", func(t *testing.T) { | ||
| var called bool | ||
| expected := &mocks.HTTPTransport{ | ||
| MockCloseIdleConnections: func() { | ||
| called = true | ||
| }, | ||
| } | ||
| netx := &Network{txp: expected} | ||
| if err := netx.Close(); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if !called { | ||
| t.Fatal("did not call the transport's CloseIdleConnections") | ||
| } | ||
| }) | ||
| } |
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
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