Port pinger command line tool

Question

Checking if some port is up on the network is a very common task when working with remote services. A common way to check is using telnet, but I have two practical issues with that:

1. telnet is not available in all systems, for example in recent versions of Windows.
2. When connection is successful with telnet, an interactive shell may start, which you have to exit by pressing Control] followed by Controld. It's OK, but not nearly as easy as running a command that simply exits with 0 on success and non-zero on failure. For the same reason, this method using telnet is not so easily scriptable.

(In case you're wondering, I excluded nmap as an alternative, because it can be used for far more than pinging ports. As far as I know, it's not recommended to have it lying around, unless you're a security expert and it's your job to use it on a daily basis.)

To solve these issues, I started a simple command line tool in go. The source code is on GitHub. I'm still a beginner of this language, I welcome any and all comments about the implementation, testing, project organization, or anything else.

The main module, portping.go:

package main

import (
    "net"
    "fmt"
    "regexp"
)

var pattern_getsockopt = regexp.MustCompile(`getsockopt: (.*)`)
var pattern_other = regexp.MustCompile(`^dial tcp: (.*)`)

func Ping(host string, port int) error {
    addr := fmt.Sprintf("%s:%d", host, port)
    conn, err := net.Dial("tcp", addr)

    if err == nil {
        conn.Close()
    }
    return err
}

func PingN(host string, port int, count int, c chan error) {
    for i := 0; i < count; i++ {
        c <- Ping(host, port)
    }
}

func FormatResult(err error) string {
    if err == nil {
        return "success"
    }
    s := err.Error()
    if result := pattern_getsockopt.FindStringSubmatch(s); result != nil {
        return result[1]
    }
    if result := pattern_other.FindStringSubmatch(s); result != nil {
        return result[1]
    }
    return s
}

Unit tests for the main module, portping_test.go:

package main

import (
    "testing"
    "fmt"
    "net"
    "log"
    "strings"
)

const testHost = "localhost"

// TODO hopefully unused. Better ideas?
const testPort = 1234

const knownNonexistentHost = "nonexistent.janosgyerik.com"

func acceptN(host string, port int, count int) {
    ln, err := net.Listen("tcp", fmt.Sprintf("%s:%d", host, port))
    if err != nil {
        log.Fatal(err)
    }
    defer ln.Close()

    for i := 0; i < count; i++ {
        conn, err := ln.Accept()
        if err != nil {
            log.Fatal(err)
        }
        conn.Close()
    }
}

func assertPingResult(host string, port int, t*testing.T, expected bool, pattern string) {
    err := Ping(host, port)

    addr := fmt.Sprintf("%s:%d", host, port)
    log.Printf("port ping %s -> %v", addr, err)

    actual := err == nil

    if expected != actual {
        var openOrClosed string
        if expected {
            openOrClosed = "open"
        } else {
            openOrClosed = "closed"
        }
        t.Errorf("%s:%d should be %s", host, port, openOrClosed)
    }

    if pattern != "" {
        errstr := err.Error()
        if !strings.Contains(errstr, pattern) {
            t.Errorf("the result was expected to contain %s, but was: %s", pattern, errstr)
        }
    }
}

func assertPingSuccess(host string, port int, t*testing.T) {
    assertPingResult(host, port, t, true, "")
}

func assertPingFailure(host string, port int, t*testing.T, pattern string) {
    assertPingResult(host, port, t, false, pattern)
}

func assertPingNSuccessCount(host string, port int, t*testing.T, pingCount int, expectedSuccessCount int) {
    c := make(chan error)
    go PingN(host, port, pingCount, c)

    addr := fmt.Sprintf("%s:%d", host, port)

    successCount := 0
    for i := 0; i < pingCount; i++ {
        err := <-c
        log.Printf("port ping %s [%d] -> %v", addr, i + 1, err)

        if err == nil {
            successCount++
        }
    }

    if expectedSuccessCount != successCount {
        t.Errorf("expected %d successful pings, but got only %d", expectedSuccessCount, successCount)
    }
}

func Test_ping_open_port(t*testing.T) {
    go acceptN(testHost, testPort, 1)

    assertPingSuccess(testHost, testPort, t)

    // for sanity: acceptN should have shut down already
    assertPingFailure(testHost, testPort, t, "connection refused")
}

func Test_ping_unopen_port(t*testing.T) {
    assertPingFailure(testHost, testPort, t, "connection refused")
}

func Test_ping_nonexistent_host(t*testing.T) {
    assertPingFailure(knownNonexistentHost, testPort, t, "no such host")
}

func Test_ping_negative_port(t*testing.T) {
    assertPingFailure(testHost, -1, t, "invalid port")
}

func Test_ping_too_high_port(t*testing.T) {
    assertPingFailure(testHost, 123456, t, "invalid port")
}

func Test_ping5_all_success(t*testing.T) {
    pingCount := 3
    go acceptN(testHost, testPort, pingCount)

    assertPingNSuccessCount(testHost, testPort, t, pingCount, pingCount)
}

func Test_ping5_all_fail(t*testing.T) {
    pingCount := 5
    successCount := 0
    assertPingNSuccessCount(testHost, testPort, t, pingCount, successCount)
}

func Test_ping5_partial_success(t*testing.T) {
    successCount := 3
    go acceptN(testHost, testPort, successCount)

    pingCount := 5
    assertPingNSuccessCount(testHost, testPort, t, pingCount, successCount)
}

func assertFormatResult(host string, port int, t*testing.T, expected string) {
    actual := FormatResult(Ping(host, port))
    if expected != actual {
        t.Errorf("expected '%s' but got '%s'", expected, actual)
    }
}

func Test_format_result_success(t*testing.T) {
    go acceptN(testHost, testPort, 1)
    assertFormatResult(testHost, testPort, t, "success")
}

func Test_format_result_connection_refused(t*testing.T) {
    assertFormatResult(testHost, testPort, t, "connection refused")
}

func Test_format_result_invalid_port_m1(t*testing.T) {
    port := -1
    assertFormatResult(testHost, port, t, fmt.Sprintf("invalid port %d", port))
}

func Test_format_result_invalid_port_123456(t*testing.T) {
    port := 123456
    assertFormatResult(testHost, port, t, fmt.Sprintf("invalid port %d", port))
}

func Test_format_result_nonexistent_host(t*testing.T) {
    host := knownNonexistentHost
    assertFormatResult(host, testPort, t, fmt.Sprintf("lookup %s: no such host", host))
}

The command line interface, main.go:

package main

import (
    "flag"
    "fmt"
    "os"
    "strconv"
)

// TODO
// flags: --tcp, --udp; default is tcp
// flag: -W timeout
// flag: -v verbose; default=false
// drop default count, print forever, until cancel with Control-C, and print stats

const defaultCount = 5

func exit() {
    flag.Usage()
    os.Exit(1)
}

type Params struct {
    host  string
    port  int
    count int
}

func parseArgs() Params {
    flag.Usage = func() {
        fmt.Printf("Usage: %s [options] host port\n\n", os.Args[0])
        flag.PrintDefaults()
    }

    countPtr := flag.Int("c", defaultCount, "stop after count connections")
    flag.Parse()

    if len(flag.Args()) < 2 {
        exit()
    }

    host := flag.Args()[0]
    port, parseErr := strconv.Atoi(flag.Args()[1])
    if parseErr != nil {
        exit()
    }

    return Params{
        host: host,
        port: port,
        count: *countPtr,
    }
}

func main() {
    params := parseArgs()

    host := params.host
    port := params.port
    count := params.count

    addr := fmt.Sprintf("%s:%d", host, port)
    fmt.Printf("Starting to ping %s ...\n", addr)

    c := make(chan error)
    go PingN(host, port, count, c)

    allSuccessful := true

    for i := 0; i < count; i++ {
        // TODO add time
        err := <-c
        if err != nil {
            allSuccessful = false
        }
        fmt.Printf("%s [%d] -> %s\n", addr, i + 1, FormatResult(err))
    }

    // TODO print summary
    // --- host:port ping statistics ---
    // n connections attempted, m successful, x% failed
    // round-trip min/avg/max/stddev = a/b/c/d ms

    if !allSuccessful {
        os.Exit(1)
    }
}

Answer 1

In general, I am impressed with the test coverage, the attention to detail, and the overall structure of the code. It is easy to follow, and understand.

Having said that, there are a few places where it can be improved.

flag processing

You code uses flag directly to set up the help mechanisms, and also for the count parameter. You should consider driving a FlagSet directly. Your code:

flag.Usage = func() {
    fmt.Printf("Usage: %s [options] host port\n\n", os.Args[0])
    flag.PrintDefaults()
}

countPtr := flag.Int("c", defaultCount, "stop after count connections")
flag.Parse()

could be reduced to:

summary := fmt.Sprintf("%s [options] host port", os.Args[0])
fs := flag.NewFlagSet(summary, flag.ExitOnError)
countPtr := fs.Int("c", defaultCount, "stop after count connections")
fs.Parse(os.Args[1:])

You will need to change a couple of flag references to fs after the above change.

I find FlagSet to be useful especially when using go-like sub-command processing.

error handling

In your code you use regular expressions to parse error messages, and simplify them for presentation.

The parsing of error messages is an anti-pattern in Go. An improved mechanism is to do type assertion on the error, and to directly manipulate the results. So, for example, the net package is documented to normally return OpError instances of an error. You can use this in your code to handle the error better (see Error handling and go blog and more specifically type switches):

func FormatResult(err error) string {
    if err == nil {
        return "success"
    }
    switch err := err.(type) {
    case *net.OpError:
        return err.Err.Error()
    default:
        return err.Error()
    }
}

Note how the type assertion err := err.(type) creates a new err instance inside the scope of the switch. The go language guarantees that the resulting err will be correctly typed in the relevant case blocks, so in this example, the case statement return err.Err.Error() is referring to the field Err on OpError.

While on the subject of error handling. It's a very common pattern in Go to have checks for NOT-NIL errors. Checks for nil errors are uncommon, and will likely be missed. Thus, this code, even though it is right, will probably be mis-read by people on the first scan:

conn, err := net.Dial("tcp", addr)
if err == nil {
    conn.Close()
}

Instead, it's common to check for an error, not the lack of one....

conn, err := net.Dial("tcp", addr)
if err != nil {
    return err
}
conn.Close()

The above code is also typically done with a "defer", but because your code does nothing else after that point, the defer would be useless, but I would still consider writing the full function as:

func Ping(host string, port int) error {
    addr := fmt.Sprintf("%s:%d", host, port)
    conn, err := net.Dial("tcp", addr)
    if err == nil {
        return err
    }
    defer conn.Close()
    return nil
}

Random ports

Heh, port 1234 - I use that port all the time for "junk" things. It seems easy.

A better solution is to choose a random, known-available port, and to then return that port as part of the call.

With TCP (and UDP) if you specify a port of 0 when you set up a listening socket, it will choose a "random" (for some degree of random), unused, ephemeral port. you can use this to your advantage.

In your tests, you use a go-routine to manage that port, but if you set up the listener before the goroutine happens, you can return the port as part of the call. So, currently your code does:

go acceptN(testHost, testPort, pingCount)

but, instead, we want something like:

port := acceptNoPortN(testHost, pingCount)

We can set that up by changing the acceptN function to something like:

func acceptNoPortN(host string, count int, t *testing.T) int {
    tcpa, err := net.ResolveTCPAddr("tcp", host+":0")
    if err != nil {
        t.Fatal(err)
    }
    ln, err := net.ListenTCP("tcp", tcpa)
    if err != nil {
        t.Fatal(err)
    }
    local, ok := ln.Addr().(*net.TCPAddr)
    if !ok {
        t.Fatalf("Unable to convert Addr to TCPAddr")
    }

    go func() {
        defer ln.Close()
        for i := 0; i < count; i++ {
            conn, err := ln.Accept()
            if err != nil {
                log.Fatal(err)
            }
            conn.Close()
        }
    }()

    return local.Port
}

Notice something in there - using the "TCP" versions of the various Dial, Resolve, and Listen methods. They help with getting instances in to the right/useful types.

The above code also changes the "String()" results in some places.... and I think that is causing some other tests to fail... maybe.

Also, it Still Has a race condition - it may not close the listener socket before the next system tries to use the port. That's OK, though, because the next attempt to do a listener should choose a different port.

Testing

In your tests, you use a combination of log.Print*(...) and also t.Errorf(....). Instead of using log.Print*(...) you should just use the t.Log*(...)

While the above is a bit messy, it's plain wrong to have log.Fatal*(...) in your tests. Use t.Fatal*(...) instead.

For what it's worth, I am not getting the tests to run.... hmmmm you have a go-routine bug in your code. This is creating a race condition:

func Test_ping_open_port(t *testing.T) {
  go acceptN(testHost, testPort, 1)

  assertPingSuccess(testHost, testPort, t)

    ...
}

The port listener is created after the assert is started. You really need to establish the socket before pinging it. My laptop is Linux, and perhaps it's quicker through the assert-side of the process than the goroutine socket listener. If I change the above to be the NoPort option, it works... mostly.

You have a different sort of bug in your assertPingResult method:

func assertPingResult(host string, port int, t *testing.T, expected bool, pattern string) {
  err := Ping(host, port)
    ....
  actual := err == nil

  if expected != actual {
        ...
      t.Errorf("%s:%d should be %s", host, port, openOrClosed)
  }

  if pattern != "" {
      errstr := err.Error()
        ....
  }
}

In the above code, if err is nil, but that's not what you expect, then you do the t.Errorf(....).... which is OK, but, if err is nil, and there's a pattern you expect, then you get a Nil pointer reference in the errstr setup.

Some other things to consider:

--- FAIL: Test_format_result_nonexistent_host (0.00s)
  main_test.go:166: expected 'lookup nonexistent.janosgyerik.com: no such host' but got 'lookup nonexistent.janosgyerik.com on
    127.0.0.1:53: no such host'
FAIL

Answer 2

The first step for the code review is to conduct a risk assessment by spending a few minutes glancing through the main.go, portping.go, and portping_test.go files. Now we know what the risks are and where to start focusing our attention.

The code is undocumented: See Godoc: documenting Go code and GoDoc.

Let's look at your Ping function.

func Ping(host string, port int) error {
    addr := fmt.Sprintf("%s:%d", host, port)
    conn, err := net.Dial("tcp", addr)

    if err == nil {
        conn.Close()
    }
    return err
}

Since it uses the net.Dial function, we glance through the net package documentation to get an idea of what the package does. We carefully read the documentation for net.Dial and related types and functions. We can also look at the net.Dial source code and tests.

func Dial

func Dial(network, address string) (Conn, error)

Dial connects to the address on the named network.

Known networks are "tcp", "tcp4" (IPv4-only), "tcp6" (IPv6-only), "udp", "udp4" (IPv4-only), "udp6" (IPv6-only), "ip", "ip4" (IPv4-only), "ip6" (IPv6-only), "unix", "unixgram" and "unixpacket".

For TCP and UDP networks, addresses have the form host:port. If host is a literal IPv6 address it must be enclosed in square brackets as in "[::1]:80" or "[ipv6-host%zone]:80". The functions JoinHostPort and SplitHostPort manipulate addresses in this form. If the host is empty, as in ":80", the local system is assumed.

Examples:

Dial("tcp", "12.34.56.78:80")
Dial("tcp", "google.com:http")
Dial("tcp", "[2001:db8::1]:http")
Dial("tcp", "[fe80::1%lo0]:80")
Dial("tcp", ":80")

type Dialer

Your Ping function signature is Ping(host string, port int) error. That won't work in all cases, including some of the examples in the net.Dial documentation, for example, host "google.com" and port "http".

You write addr := fmt.Sprintf("%s:%d", host, port). The net.Dial documentation states that the function JoinHostPort manipulates addresses in this form. No employer wants employees who write idiosyncratic, unreadable, and unmaintainable code.

If the dialed connection can't be made net.Dial waits, and waits, and waits forever. When we read through the net package documentation we noticed that there is a DialTimeout function. Let's use that.

func DialTimeout

func DialTimeout(network, address string, timeout time.Duration) (Conn, error)

DialTimeout acts like Dial but takes a timeout. The timeout includes name resolution, if required.

In the net package documentation and everywhere else in the Go standard library we have

The Dial function connects to a server:

conn, err := net.Dial("tcp", "google.com:80")
if err != nil {
  // handle error
}

The Listen function creates servers:

ln, err := net.Listen("tcp", ":8080")
if err != nil {
  // handle error
}

You idiosyncratically choose the reverse form

if err == nil {
    conn.Close()
}
return err

Go Code Review Comments

Indent Error Flow

Try to keep the normal code path at a minimal indentation, and indent the error handling, dealing with it first. This improves the readability of the code by permitting visually scanning the normal path quickly. For instance, don't write:

if err != nil {
    // error handling
} else {
    // normal code
}

Instead, write:

if err != nil {
    // error handling
    return // or continue, etc.
}
// normal code

You only close the connection if there is no error returned!

if err == nil {
    conn.Close()
}

Always close the connection.

conn, err := net.DialTimeout(a.Network(), a.String(), 10*time.Second)
defer conn.Close()
if err != nil {
    return err
}

We know that this is safe to do (a nil value for conn is OK) and that it is the expected behaviour because we read the net.Dial source code.

src/net/net.go

func (c *conn) ok() bool { return c != nil && c.fd != nil }

// Close closes the connection.
func (c *conn) Close() error {
  if !c.ok() {
      return syscall.EINVAL
  }
  err := c.fd.Close()
  if err != nil {
      err = &OpError{Op: "close", Net: c.fd.net, Source: c.fd.laddr, Addr: c.fd.raddr, Err: err}
  }
  return err
}

Here's a first draft of how I might write an idiomatic Go Ping function.

// Ping dials a TCP network address and waits for a connection.
func Ping(host, port string) error {
    addr := net.JoinHostPort(host, port)
    a, err := net.ResolveTCPAddr("tcp", addr)
    if err != nil {
        return err
    }
    conn, err := net.DialTimeout(a.Network(), a.String(), 10*time.Second)
    defer conn.Close()
    if err != nil {
        return err
    }
    return nil
}

I see other problems with your code.