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

[PeerList] Refactor Peer Start/Finish functions for Peers && Add finish closure to PeerLists #472

Closed
willhug opened this issue Nov 22, 2016 · 3 comments
Assignees
Milestone

Comments

@willhug
Copy link
Contributor

willhug commented Nov 22, 2016

Peer Interface Old world:

StartRequest() (finish func())

Peer Interface New World:

StartRequest(Subscriber) // Subscriber is the subscriber that *Shouldn't* be notified
FinishRequest(Subscriber)  // Subscriber is the subscriber that *Shouldn't* be notified

PeerList old ChoosePeer func:

ChoosePeer(context.Context, *transport.Request) (Peer, error)

PeerList New ChoosePeer func:

// ChoosePeer should run the Peer.StartRequest and Peer.FinishRequest (through it's onFinish closure)
ChoosePeer(context.Context, *transport.Request) (p Peer, onFinish func(error), error)

This will allow the PeerLists to safely update their own internal state (within a lock) and remove the opportunity for a deadlock from the Notify.

@willhug willhug self-assigned this Nov 22, 2016
@willhug willhug added this to the 1.0.0 milestone Nov 22, 2016
@abhinav
Copy link
Contributor

abhinav commented Nov 22, 2016

StartRequest(Subscriber) // Subscriber is the subscriber that *Shouldn't* be
notified
FinishRequest(Subscriber)  // Subscriber is the subscriber that *Shouldn't* be notified

That's a weird interface. StartRequest(Subscriber) sounds like the complete
opposite of that.

@willhug
Copy link
Contributor Author

willhug commented Dec 1, 2016

This is half finished now after @kriskowal 's change. The only change left is updating the ChoosePeer format to return the "on finish" method

@kriskowal
Copy link
Contributor

Prioritizing for RC 3, @willhug

willhug added a commit that referenced this issue Dec 8, 2016
Summary: in #472 we defined a new interface for having safer onfinish changes. This diff performs the change to the interface
@willhug willhug closed this as completed Dec 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants