Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reliable.DispatcherService use context instead of 2 methods #3402

Closed
lukedirtwalker opened this issue Nov 19, 2019 · 4 comments · Fixed by #3441
Closed

reliable.DispatcherService use context instead of 2 methods #3402

lukedirtwalker opened this issue Nov 19, 2019 · 4 comments · Fixed by #3441
Assignees
Labels
refactor Change that focuses around reducing tech debt
Milestone

Comments

@lukedirtwalker
Copy link
Collaborator

reliable.DispatcherService interface has 2 methods on without timeout one with timeout. We should refactor to a single method which takes a context as first argument.

@lukedirtwalker lukedirtwalker added the refactor Change that focuses around reducing tech debt label Nov 19, 2019
@scrye scrye added this to the Q4S4.D milestone Nov 25, 2019
@scrye
Copy link
Contributor

scrye commented Nov 25, 2019

If revisiting this code, also removing Bind Address from the API would be useful.

@scrye
Copy link
Contributor

scrye commented Nov 25, 2019

Maybe ctxAwareConnect in go/lib/sciond is an useful starting point.

@karampok
Copy link
Contributor

What do you think:

From


	Register(ia addr.IA, public *addr.AppAddr, bind *net.UDPAddr,
		svc addr.HostSVC) (net.PacketConn, uint16, error)
	RegisterTimeout(ia addr.IA, public *addr.AppAddr, bind *net.UDPAddr,
		svc addr.HostSVC, timeout time.Duration) (net.PacketConn, uint16, error)

to

Register1(context.Context, addr.IA, *net.UDPAddr, addr.HostSVC) (net.PacketConn, uint16, error)
Register2(context.Context, net.Addr) (net.PacketConn, uint16, error)

@scrye @lukedirtwalker

@scrye
Copy link
Contributor

scrye commented Nov 28, 2019

I think we can simplify even more. In #3136 (comment) I suggested:

type Dispatcher interface {
    Register(ctx context.Context, registration *snet.UDPAddr, svc addr.HostSVC) (PacketConn, *snet.UDPAddr, error)
}

I think there are a few improvements that we can make to that suggestion.

@matzf also suggested we make svc a pointer, since that way it's clearer that a user doesn't want to register for SVCs.

Also, I think the returned address is superfluous; it can be extracted easily via net.PacketConn.LocalAddr. Also, the address type that we register can be whatever; it's the implementation's job to test that it's supported, and return an error if that is not the case (that is, for what we have right now in go/lib/sock/reliable, type assert at registration time and see if it can get a *snet.UDPAddr).

Putting it all together, we get:

Register(context.Context, net.Addr, *addr.HostSVC) (net.PacketConn, error)

What are your thoughts about ^?

Also, reliable.DispatcherService should move to snet; moving it where it is used instead of where it is implemented eliminates the risk of import loops, and makes the type ecosystem more straightforward in general (all the basic tools a user interacts with live in snet, there implementations may live wherever).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Change that focuses around reducing tech debt
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants