dht find connected peers #428
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package dht | |
|
|
||
| import ( | ||
| "bytes" | ||
| "sort" | ||
| "testing" | ||
|
|
||
| context "github.com/jbenet/go-ipfs/Godeps/_workspace/src/code.google.com/p/go.net/context" | ||
|
|
@@ -64,6 +65,14 @@ func setupDHTS(ctx context.Context, n int, t *testing.T) ([]ma.Multiaddr, []peer | |
| return addrs, peers, dhts | ||
| } | ||
|
|
||
| func makePeerString(t *testing.T, addr string) peer.Peer { | ||
| maddr, err := ma.NewMultiaddr(addr) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| return makePeer(maddr) | ||
| } | ||
|
|
||
| func makePeer(addr ma.Multiaddr) peer.Peer { | ||
| sk, pk, err := ci.GenerateKeyPair(ci.RSA, 512) | ||
| if err != nil { | ||
|
|
@@ -406,6 +415,100 @@ func TestFindPeer(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestFindPeersConnectedToPeer(t *testing.T) { | ||
| if testing.Short() { | ||
| t.SkipNow() | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| u.Debug = false | ||
|
There was a problem hiding this comment. prev := u.Debug
u.Debug = false
defer u.Debug = prev? There was a problem hiding this comment. Woah what. sorry idk how that got in there. There was a problem hiding this comment. huh, it's all over the place in this file. dont remember why. @whyrusleeping any idea? There was a problem hiding this comment.
lovely is this pattern >> http://commandcenter.blogspot.nl/2014/01/self-referential-functions-and-design.html There was a problem hiding this comment. merging. we'll solve this later. #430 |
||
|
|
||
| _, peers, dhts := setupDHTS(ctx, 4, t) | ||
| defer func() { | ||
| for i := 0; i < 4; i++ { | ||
| dhts[i].Close() | ||
| dhts[i].dialer.(inet.Network).Close() | ||
| } | ||
| }() | ||
|
|
||
| // topology: | ||
| // 0-1, 1-2, 1-3, 2-3 | ||
| err := dhts[0].Connect(ctx, peers[1]) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| err = dhts[1].Connect(ctx, peers[2]) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| err = dhts[1].Connect(ctx, peers[3]) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| err = dhts[2].Connect(ctx, peers[3]) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // fmt.Println("0 is", peers[0]) | ||
| // fmt.Println("1 is", peers[1]) | ||
| // fmt.Println("2 is", peers[2]) | ||
| // fmt.Println("3 is", peers[3]) | ||
|
|
||
| ctxT, _ := context.WithTimeout(ctx, time.Second) | ||
| pchan, err := dhts[0].FindPeersConnectedToPeer(ctxT, peers[2].ID()) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| // shouldFind := []peer.Peer{peers[1], peers[3]} | ||
| found := []peer.Peer{} | ||
| for nextp := range pchan { | ||
| found = append(found, nextp) | ||
| } | ||
|
|
||
| // fmt.Printf("querying 0 (%s) FindPeersConnectedToPeer 2 (%s)\n", peers[0], peers[2]) | ||
| // fmt.Println("should find 1, 3", shouldFind) | ||
| // fmt.Println("found", found) | ||
|
|
||
| // testPeerListsMatch(t, shouldFind, found) | ||
|
|
||
| log.Warning("TestFindPeersConnectedToPeer is not quite correct") | ||
| if len(found) == 0 { | ||
| t.Fatal("didn't find any peers.") | ||
| } | ||
| } | ||
|
|
||
| func testPeerListsMatch(t *testing.T, p1, p2 []peer.Peer) { | ||
|
|
||
| if len(p1) != len(p2) { | ||
| t.Fatal("did not find as many peers as should have", p1, p2) | ||
| } | ||
|
|
||
| ids1 := make([]string, len(p1)) | ||
| ids2 := make([]string, len(p2)) | ||
|
|
||
| for i, p := range p1 { | ||
| ids1[i] = p.ID().Pretty() | ||
| } | ||
|
|
||
| for i, p := range p2 { | ||
| ids2[i] = p.ID().Pretty() | ||
| } | ||
|
|
||
| sort.Sort(sort.StringSlice(ids1)) | ||
| sort.Sort(sort.StringSlice(ids2)) | ||
|
|
||
| for i := range ids1 { | ||
| if ids1[i] != ids2[i] { | ||
| t.Fatal("Didnt find expected peer", ids1[i], ids2) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestConnectCollision(t *testing.T) { | ||
| if testing.Short() { | ||
| t.SkipNow() | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is programmer error. It's not an error that should be allowed at runtime / handled. variable addresses should be addressed long before we get here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more generally, i'm coming to think that returning
errall over the place can actually lead to harder to debug things than well understood panics. it treatserroras a catch-all, instead of purposeful error handling of things known to go wrong. imagine if index errors returnederrin slices? the stdlib panics all over the place to signal what we definitely shouldnt do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not the errors themselves that cause that. it's masking them behind log statements and
continuingover them. those practices will certainly make it harder to debug behavior.i'm in complete disagreement regarding panics versus erroring. but whatever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#412 documents a panic that takes down the daemon during
ipfs swarm connect. i weep when i see more opportunities for panicking going into subsystems.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a separate issue, and yes, if that's used incorrectly it's pretty bad. In servers though, logging the error often IS exactly what we want to do (think 404s, 500s, etc).
errshould be handled correctly OR gracefully bubble back to {exit the application in cli, exit the request in server}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean to call attention to the fact the this separate issue is the actual reason why one would begin to lose confidence in errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that adding errors all over the place is bad practice itself. Look at the stdlib: https://gist.github.com/jbenet/9c20705c43a7ae47f9be Programmer error that is always incorrect, should not ship, should panic. We disagree on this and i dont think we'll reach consensus now.
And yeah, #412 is a problem that needs to be addressed.