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

dht dynamic mode switching. #367

Closed
wants to merge 5 commits into from
Closed

Conversation

raulk
Copy link
Member

@raulk raulk commented Jun 27, 2019

We now listen to local routability events to dynamically switch our DHT mode.

Private/unknown routability transition us to client mode; public routability puts us in server mode.

Fixes #216.

We now listen to local routability events to dynamically switch our DHT
mode. Private/unknown routability transition us to client mode; public
routability puts us in server mode.
@raulk raulk requested a review from whyrusleeping June 27, 2019 15:48
dht.go Outdated Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
Copy link
Contributor

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems about right to me.

@raulk
Copy link
Member Author

raulk commented Jun 28, 2019

@Stebalien @whyrusleeping debouncing implemented. Can I get a quick sanity check?

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at #369.

dht.go Show resolved Hide resolved
dht.go Outdated Show resolved Hide resolved
@raulk
Copy link
Member Author

raulk commented Jun 28, 2019

Do not review yet, I'm cleaning this up.

}
select {
case <-debouncer.C:
default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this is technically kind of racy because Stop doesn't wait for the timer to stop, but it shouldn't really be an issue.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timer.Stop() is a huge headache. I never got it to work as described in godocs, so I went this route 🙃

@Stebalien
Copy link
Member

@raulk I made dynamicModeSwitching just use goprocess.Go directly. Feel free to blow that away if you're refactoring (we can just redo it).

@raulk raulk force-pushed the feat/dht-dynamic-mode-switching branch from 6533504 to 68686f3 Compare June 28, 2019 16:58
@raulk
Copy link
Member Author

raulk commented Jun 28, 2019

@raulk I made dynamicModeSwitching just use goprocess.Go directly. Feel free to blow that away if you're refactoring (we can just redo it).

@Stebalien exactly the refactoring I was doing. I went a little further and fixed subscriberNotifee too to use the same approach. I also moved the subscriptions to local scope.

Still LGTY?

dht := nn.DHT()

proc := goprocess.Go(nn.subscribe)
dht.host.Network().Notify(nn)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly concerned that this will be triggered async. We can register for notifications after we receive connections.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this applies to the other subscribers as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Stebalien Please can you elaborate on this ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember... I think my concern was that we could listen for new connections after the network has started and we already have connections, but that's already the case.

@jacobheun jacobheun added this to the Working Kademlia milestone Jan 23, 2020
@jacobheun jacobheun linked an issue Feb 6, 2020 that may be closed by this pull request
@aschmahmann
Copy link
Contributor

@aarshkshah1992 it looks like you're taking another stab at this PR. I don't think that the debouncer stuff necessarily makes a ton of sense, it should (and to some extent already does) exist in autonat. Adding a separate one for the DHT seems a little strange.

I ported some of these changes into my DHT branch (35d3e4a). I wasn't sure what was going on with the peer event changes removal changes in this PR so I left it alone.

@aarshkshah1992
Copy link
Contributor

Code from this PR has been absorbed in #436 & #448 combined. This PR can now be closed.

@Stebalien Stebalien closed this Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an "Auto" client/server DHT mode
6 participants