Skip to content
New issue

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

#

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

Already on GitHub? # to your account

cleanup the ping command #5680

Merged
merged 3 commits into from
Oct 30, 2018
Merged

cleanup the ping command #5680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b870e8c
cleanup and improve ping
Stebalien Oct 27, 2018
b8f5355
simplify ping.ParsePeerParam
Stebalien Oct 27, 2018
62240f3
sharness: verify that we return an error when we can't lookup a peer …
Stebalien Oct 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 58 additions & 110 deletions core/commands/ping.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ import (
"strings"
"time"

"github.com/ipfs/go-ipfs/core"
"github.com/ipfs/go-ipfs/core/commands/cmdenv"

cmds "gx/ipfs/QmSXUokcP4TJpFfqozT69AVAYRtzXVMUjzQVkYX41R9Svs/go-ipfs-cmds"
ma "gx/ipfs/QmT4U94DnD8FRfqr21obWY32HLM5VExccPKMjQHofeYqr9/go-multiaddr"
"gx/ipfs/QmTRhk7cgjUf2gfQ3p2M9KPECNZEW9XUrmHcFCgog4cPgB/go-libp2p-peer"
pstore "gx/ipfs/QmTTJcDL3gsnGDALjh2fDGg1onGRUdVgNL2hU2WEZcVrMX/go-libp2p-peerstore"
iaddr "gx/ipfs/QmZc5PLgxW61uTPG24TroxHDF6xzgbhZZQf5i53ciQC47Y/go-ipfs-addr"
cmdkit "gx/ipfs/Qmde5VP1qUkyQXKCfmEUA7bP64V2HAptbJ7phuPp7jXWwg/go-ipfs-cmdkit"
)

Expand Down Expand Up @@ -59,166 +59,114 @@ trip latency information.
return ErrNotOnline
}

addr, peerID, err := ParsePeerParam(req.Arguments[0])
addr, pid, err := ParsePeerParam(req.Arguments[0])
if err != nil {
return fmt.Errorf("failed to parse peer address '%s': %s", req.Arguments[0], err)
}

if peerID == n.Identity {
if pid == n.Identity {
return ErrPingSelf
}

if addr != nil {
n.Peerstore.AddAddr(peerID, addr, pstore.TempAddrTTL) // temporary
n.Peerstore.AddAddr(pid, addr, pstore.TempAddrTTL) // temporary
}

numPings, _ := req.Options[pingCountOptionName].(int)
if numPings <= 0 {
return fmt.Errorf("error: ping count must be greater than 0, was %d", numPings)
}

outChan := pingPeer(req.Context, n, peerID, numPings)

return res.Emit(outChan)
},
Type: PingResult{},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, out *PingResult) error {
if len(out.Text) > 0 {
fmt.Fprintln(w, out.Text)
} else if out.Success {
fmt.Fprintf(w, "Pong received: time=%.2f ms\n", out.Time.Seconds()*1000)
} else {
fmt.Fprintf(w, "Pong failed\n")
}
return nil
}),
},
}

func pingPeer(ctx context.Context, n *core.IpfsNode, pid peer.ID, numPings int) <-chan interface{} {
outChan := make(chan interface{})
go func() {
defer close(outChan)

if len(n.Peerstore.Addrs(pid)) == 0 {
// Make sure we can find the node in question
select {
case outChan <- &PingResult{
if err := res.Emit(&PingResult{
Text: fmt.Sprintf("Looking up peer %s", pid.Pretty()),
Success: true,
}:
case <-ctx.Done():
return
}); err != nil {
return err
}

ctx, cancel := context.WithTimeout(ctx, kPingTimeout)
defer cancel()
ctx, cancel := context.WithTimeout(req.Context, kPingTimeout)
p, err := n.Routing.FindPeer(ctx, pid)
cancel()
if err != nil {
select {
case outChan <- &PingResult{Text: fmt.Sprintf("Peer lookup error: %s", err)}:
case <-ctx.Done():
Copy link
Member Author

Choose a reason for hiding this comment

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

This should have used the command context, not the find-peer context. This context is pretty much guaranteed to be expired at this point (so will return "success" to the user when we fail to lookup the peer). This is causing the JS interop tests to fail (for good reason).

return
}

return
return res.Emit(&PingResult{Text: fmt.Sprintf("Peer lookup error: %s", err)})
}
n.Peerstore.AddAddrs(p.ID, p.Addrs, pstore.TempAddrTTL)
}

select {
case outChan <- &PingResult{
if err := res.Emit(&PingResult{
Text: fmt.Sprintf("PING %s.", pid.Pretty()),
Success: true,
}:
case <-ctx.Done():
return
}); err != nil {
return err
}

ctx, cancel := context.WithTimeout(ctx, kPingTimeout*time.Duration(numPings))
ctx, cancel := context.WithTimeout(req.Context, kPingTimeout*time.Duration(numPings))
defer cancel()
pings, err := n.Ping.Ping(ctx, pid)
if err != nil {
select {
case outChan <- &PingResult{
return res.Emit(&PingResult{
Success: false,
Text: fmt.Sprintf("Ping error: %s", err),
}:
case <-ctx.Done():
}
return
})
}

var done bool
var total time.Duration
for i := 0; i < numPings && !done; i++ {
ticker := time.NewTicker(time.Second)
defer ticker.Stop()
for i := 0; i < numPings; i++ {
t, ok := <-pings
if !ok {
break
}

if err := res.Emit(&PingResult{
Success: true,
Time: t,
}); err != nil {
return err
}

total += t

select {
case <-ticker.C:
case <-ctx.Done():
done = true
break
case t, ok := <-pings:
if !ok {
done = true
break
}
select {
case outChan <- &PingResult{
Success: true,
Time: t,
}:
case <-ctx.Done():
return
}
total += t
time.Sleep(time.Second)
return ctx.Err()
}
}
averagems := total.Seconds() * 1000 / float64(numPings)
select {
case outChan <- &PingResult{
return res.Emit(&PingResult{
Success: true,
Text: fmt.Sprintf("Average latency: %.2fms", averagems),
}:
case <-ctx.Done():
}
}()
return outChan
})
},
Type: PingResult{},
Encoders: cmds.EncoderMap{
cmds.Text: cmds.MakeTypedEncoder(func(req *cmds.Request, w io.Writer, out *PingResult) error {
if len(out.Text) > 0 {
fmt.Fprintln(w, out.Text)
} else if out.Success {
fmt.Fprintf(w, "Pong received: time=%.2f ms\n", out.Time.Seconds()*1000)
} else {
fmt.Fprintf(w, "Pong failed\n")
}
return nil
}),
},
}

func ParsePeerParam(text string) (ma.Multiaddr, peer.ID, error) {
// to be replaced with just multiaddr parsing, once ptp is a multiaddr protocol
idx := strings.LastIndex(text, "/")
if idx == -1 {
pid, err := peer.IDB58Decode(text)
if err != nil {
return nil, "", err
}

return nil, pid, nil
}

addrS := text[:idx]
peeridS := text[idx+1:]

var maddr ma.Multiaddr
var pid peer.ID

// make sure addrS parses as a multiaddr.
if len(addrS) > 0 {
var err error
maddr, err = ma.NewMultiaddr(addrS)
// Multiaddr
if strings.HasPrefix(text, "/") {
a, err := iaddr.ParseString(text)
if err != nil {
return nil, "", err
}
return a.Transport(), a.ID(), nil
}

// make sure idS parses as a peer.ID
var err error
pid, err = peer.IDB58Decode(peeridS)
if err != nil {
return nil, "", err
}

return maddr, pid, nil
// Raw peer ID
p, err := peer.IDB58Decode(text)
return nil, p, err
}
9 changes: 9 additions & 0 deletions test/sharness/t0041-ping.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ test_description="Test ping command"

test_init_ipfs

BAD_PEER="QmNnooDu7bfjPFoTZYxMNLWUQJyrVwtbZg5gBMjTezGAJx"

# start iptb + wait for peering
test_expect_success 'init iptb' '
iptb init -n 2 --bootstrap=none --port=0
Expand All @@ -23,6 +25,13 @@ test_expect_success "test ping other" '
ipfsi 1 ping -n2 -- "$PEERID_0"
'

test_expect_success "test ping unreachable peer" '
printf "Looking up peer %s\n" "$BAD_PEER" > bad_ping_exp &&
printf "Peer lookup error: routing: not found\n" >> bad_ping_exp &&
ipfsi 0 ping -n2 -- "$BAD_PEER" > bad_ping_actual &&
test_cmp bad_ping_exp bad_ping_actual
'

test_expect_success "test ping self" '
! ipfsi 0 ping -n2 -- "$PEERID_0" &&
! ipfsi 1 ping -n2 -- "$PEERID_1"
Expand Down