Remove path resolution step from snet writes #3571
Conversation
There was a problem hiding this comment.
Just a question: does this mean that in the future a user of snet must choose a path before writing?
Reviewable status: 0 of 2 files reviewed, all discussions resolved
c6fa34f to
d5db301
Compare
|
@matzf yes after this PR, user must always complete the path. Any objection use case for that? |
d5db301 to
51ec60e
Compare
There was a problem hiding this comment.
@matzf yes after this PR, user must always complete the path. Any objection use case for that?
Not an objection, just asking for clarification. One thing I don't understand is how an application can now Dial a long lived connection -- paths expire after some time, and after Dial the path can not be updated. If I'm not mistaken, not specifying a path would previously query for a fresh path for each packet so this would have worked for long lived connections.
Reviewable status: 0 of 11 files reviewed, all discussions resolved (waiting on @scrye)
There was a problem hiding this comment.
There are a few tricky problems with doing path resolution inside a conn, which is why I think it's premature to support it correctly in snet:
context/deadline interaction inside the conn is hard to reason about. Setting a deadline in the past on a conn needs to guarantee that all reads/writes exit, meaning the conn object needs to track all ongoing path request contexts and cancel them. An even harder problem is setting a deadline in the future that is prior to the old deadline after acontext.Contexthas been passed into a path request. Should a cancellation timer be armed in this case?- doing one path request per packet is expensive, so some layer of path caching needs to be implemented; some caching strategies might not be desired by the client, so the API needs to give some form of control to the caller;
- (probably the most important one) we realized most applications (other than a few simple tests) never used it anyway, so we can do away with most of the complexity above.
For long lived connection objects, a solution would be to implement a Dial on top of snet that internally calls snet.Listen but also has a path manager of the caller's choice to get paths. The caller instantiating that path manager means they can customize caching behavior, and decide how they want contexts and deadlines to interact (maybe they don't care about implementing net.Conn, so they don't need the heavyweight context tracking). The SIG is an example of such an application. The Messenger's RPCs also work in a sort of similar way.
I can see a long lived Dial becoming a thing in the future, but supporting it in the underlying core of snet feels premature at the moment.
Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karampok)
a discussion (no related file):
Expand the commit message to include the reason for removal, and maybe rephrase the title to something more descriptive. For example:
Remove path resolution step from snet writes
Resolving paths in snet writes requires complex interactions between contexts and deadlines. However, most SCION applications never used this feature, instead opting to manage paths themselves (see CS, PS, SIG). Removing path resolution from writes makes snet simpler to reason about and maintain.
a discussion (no related file):
Mark the PR as a breaking change. It's not for applications in this repo, but it will break external applications that relied on this feature.
a discussion (no related file):
Change the docstring of Dial to say that the remote address requires a path and underlay next hop to be set if the destination is in a remote AS.
Also mention that the underlay next hop is inferred if the destination is in the local AS.
go/lib/snet/writer.go, line 121 at r1 (raw file):
func (c *scionConnWriter) SetWriteDeadline(t time.Time) error { if err := c.conn.SetWriteDeadline(t); err != nil {
You can reduce the method to one line now:
return c.conn.SetWriteDeadline(t)
51ec60e to
b506fb8
Compare
There was a problem hiding this comment.
Reviewable status: 10 of 12 files reviewed, 4 unresolved discussions (waiting on @karampok and @scrye)
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Mark the PR as a breaking change. It's not for applications in this repo, but it will break external applications that relied on this feature.
Done.
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Change the docstring of
Dialto say that the remote address requires a path and underlay next hop to be set if the destination is in a remote AS.Also mention that the underlay next hop is inferred if the destination is in the local AS.
Done.
a discussion (no related file):
Previously, scrye (Sergiu Costea) wrote…
Expand the commit message to include the reason for removal, and maybe rephrase the title to something more descriptive. For example:
Remove path resolution step from snet writes
Resolving paths in snet writes requires complex interactions between contexts and deadlines. However, most SCION applications never used this feature, instead opting to manage paths themselves (see CS, PS, SIG). Removing path resolution from writes makes
snetsimpler to reason about and maintain.
Done.
go/lib/snet/writer.go, line 121 at r1 (raw file):
Previously, scrye (Sergiu Costea) wrote…
You can reduce the method to one line now:
return c.conn.SetWriteDeadline(t)
Done.
Resolving paths in snet writes requires complex interactions between contexts and deadlines. However, most SCION applications never used this feature, instead opting to manage paths themselves (see CS, PS, SIG). Removing path resolution from writes makes snet simpler to reason about and maintain.
b506fb8 to
fac5381
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
Previously, snet would resolve a path every in every write to an snet.Conn if no path was specified as part of the destination address. This has been changed in scionproto/scion#3571, now a path is required if the destination is not in the same AS. This has some implications on long lived connections and will require a path management layer on top to make this work again.
Minimal fix for `snet.Conn.Write`. Previously, passing `nil` into `WriteTo` would automatically pick the connected remote address, if available. With some of the previous refactoring (amongst other, in scionproto#3571), this is no longer the case. Right now, `Write` is simply broken. Apply the simplest fix, just specify to use the connected address (`base.remote`) in `Write`. Note that the behaviour is still subtly different than it used to be (e.g. no error if WriteTo is invoked on a connected socket), but this seems to be the accepted behaviour change within the refactoring in scionproto#3571. Additionally, add a specific error message for the `nil` address case in `WriteTo`, as "Unable to write to non-SCION address" was misleading.
* Fix connected Write on snet.Conn Minimal fix for `snet.Conn.Write`. Previously, passing `nil` into `WriteTo` would automatically pick the connected remote address, if available. With some of the previous refactoring (amongst other, in #3571), this is no longer the case. Right now, `Write` is simply broken. Apply the simplest fix, just specify to use the connected address (`base.remote`) in `Write`. Note that the behaviour is still subtly different than it used to be (e.g. no error if WriteTo is invoked on a connected socket), but this seems to be the accepted behaviour change within the refactoring in #3571. Additionally, add a specific error message for the `nil` address case in `WriteTo`, as "Unable to write to non-SCION address" was misleading. * Fix type switch style
Add wrapper for snet initialisation and connection establishment, called `appnet`. This wrapper takes care of determining the local address, so we don't have to specify this when invoking the applications. It does this, by querying sciond for the local IA and by querying the hosts routing table for the appropriate local address. This works without the previously employed workarounds of having a `localhost` entry in `/etc/hosts` or the `gen/ia` file. The wrapper hides away the choice of the dispatcher/sciond socket from the applications by moving this choice to an environment variable -- this will only be necessary in runs on the local-topology This will make usage simpler and more uniform across applications. Also, `appnet.Dial`/`DialAddr` take care of always setting a path to the remote address, using the (new) helper function `appnet.SetDefaultPath(addr)`. This is required after scionproto/scion#3571. Previously, snet would resolve a path every in every write to an snet.Conn if no path was specified as part of the destination address. Now a path needs to be set on the address if the destination is not in the same AS. Note: this has some implications on long lived connections and will later require a path management layer on top to make this work again. Additionally, using the `appnet.Dial` and `appquic.Dial` will resolve hostnames, so that now all apps uniformly support using hostnames as arguments. Note: a _copy_ of this `appnet` wrapper code will also be used in RAINS (netsec-ethz/rains#231), for the SCION transport in rainsd, rdig and the library. The duplication is obviously undesirable, but the library in scion-apps wants to make use of RAINS to resolve names, so there does not seem to be a way around this without having a third, shared library -- having a shared library just for this would seem to be overly complicated for this. Also in this PR: * collate examples in `_examples/` subdirectory, following "standard" golang directory layout. Only keep one client/server pair for shttp examples * rename lib/ to pkg/ as first step towards better project layout * helloworld sample application simplified, making use of the `appnet` wrapper. Add a server mode that prints the message sent to it * improve Makefile (all targets PHONY, build from root directory to get proper paths in error msg) * add `make test` target. Disable failing internal tests copied from `bat` clone. * add `make lint` target using `golangci-lint`. Configured to ignore most of `bat` internals. Also `webapp` and `bwtester` have too many linting to be fixed right now. * fix a ton of small lint issues. * set up CircleCI to run tests and lint * Fix & combine shttp examples, improve/fix shttp URL mangling Now shttp offers a helper function MangleSCIONAddrURL that can be used as a filter to mangle SCION addresses in URLs, before passing them to URL.Parse etc. Use this in bat and the example client code to allow using SCION addresses directly. * Reorganize reading /etc/hosts to fix tests Reorganize reading /etc/hosts as this was broken in test set up (would read both the actual /etc/hosts and the test file). In the process, change GetNamesByAddress signature take address type not string (fixes canonicalization problem with string only comparison).
Resolving paths in snet writes requires complex interactions between contexts and deadlines. However, most SCION applications never used this feature, instead opting to manage paths themselves (see CS, PS, SIG). Removing path resolution from writes makes snet simpler to reason about and maintain.
This change is