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

refactor(kademlia): add peers at start asynchronously #2082

Merged
merged 1 commit into from
Jun 16, 2021
Merged

Conversation

mrekucci
Copy link
Contributor

@mrekucci mrekucci commented Jun 14, 2021

This change is Reviewable

@mrekucci mrekucci requested review from istae and acud June 14, 2021 14:40
@mrekucci mrekucci added the ready for review The PR is ready to be reviewed label Jun 14, 2021
@mrekucci mrekucci removed the ready for review The PR is ready to be reviewed label Jun 14, 2021
@ldeffenb
Copy link
Collaborator

ldeffenb commented Jun 14, 2021

This may or may not improve anything. In my experience, the /topology Debug API also doesn't get an answer until the AddPeers call completes. So there seems to be some locking going on somewhere that may still prevent the Kademlia from making peer connection progress.

Give it a try sometime. Start up a peer with a large (300,000+ population) address book and try to get :1635/topology. It will sit there for a long time (15-30 minutes for my nodes) until it responds. And as soon as it responds, the peer connections start happening.

Other Debug APIs like /balances /addresses /settlements work perfectly fine during this delay.

Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @anatollupacescu, @esadakar, and @mrekucci)


pkg/p2p/libp2p/libp2p.go, line 275 at r1 (raw file):

	select {
	case <-s.ready:
		go func() { _ = stream.Reset() }()

you would need to return after this, since this would just continue execution here


pkg/topology/kademlia/kademlia.go, line 585 at r1 (raw file):

		addresses, err := k.addressBook.Overlays()
		if err != nil {
			panic(fmt.Errorf("addressbook overlays: %w", err))

I'm not sure that panicking is the right thing to do here, as it would cause a dirty shutdown. Logging the error and returning should be fine

Copy link
Contributor Author

@mrekucci mrekucci left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @acud, @anatollupacescu, and @esadakar)


pkg/topology/kademlia/kademlia.go, line 585 at r1 (raw file):

Previously, acud (acud) wrote…

I'm not sure that panicking is the right thing to do here, as it would cause a dirty shutdown. Logging the error and returning should be fine

Done.


pkg/p2p/libp2p/libp2p.go, line 275 at r1 (raw file):

Previously, acud (acud) wrote…

you would need to return after this, since this would just continue execution here

Getting rid of this since the nodes cannot connect to each other even with the return statement.

@mrekucci mrekucci added the ready for review The PR is ready to be reviewed label Jun 16, 2021
Copy link
Member

@acud acud left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @anatollupacescu and @esadakar)

@mrekucci mrekucci merged commit 0503716 into master Jun 16, 2021
@mrekucci mrekucci deleted the fix-kad-start branch June 16, 2021 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pull-request ready for review The PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants