From 34cdfa258b34b180982f611e027938cbc21c6cc9 Mon Sep 17 00:00:00 2001 From: Tom Barker Date: Thu, 19 Aug 2021 10:36:07 -0400 Subject: [PATCH 1/4] Test_Connections currently fails intermittently on Linux (and maybe other OSs), and fails consistently if run with `go test -times=N` On inspection, Go closes TCP connections when they go out of scope and are garbage collected. I've re-written Test_Connections() to explicitly close connectections once the test has finished. This has the other benefit of closing gracefully, which means the -times argument should work. I've also removed the t.Skip() calls inside goroutines as they are unsupported. --- process/process_test.go | 58 +++++++++++++++++++++++++--------------------- v3/process/process_test.go | 58 +++++++++++++++++++++++++--------------------- 2 files changed, 62 insertions(+), 54 deletions(-) diff --git a/process/process_test.go b/process/process_test.go index 5eaa039..430ea50 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -498,46 +498,50 @@ func Test_Parent(t *testing.T) { func Test_Connections(t *testing.T) { p := testGetProcess() - ch0 := make(chan string) - ch1 := make(chan string) + + addr, err := net.ResolveTCPAddr("tcp", "localhost:0") // dynamically get a random open port from OS + if err != nil { + t.Fatalf("unable to resolve localhost: %v", err) + } + l, err := net.ListenTCP(addr.Network(), addr) + if err != nil { + t.Fatalf("unable to listen on %v: %v", addr, err) + } + defer l.Close() + + tcpServerAddr := l.Addr().String() + tcpServerAddrIP := strings.Split(tcpServerAddr, ":")[0] + tcpServerAddrPort, err := strconv.ParseUint(strings.Split(tcpServerAddr, ":")[1], 10, 32) + if err != nil { + t.Fatalf("unable to parse tcpServerAddr port: %v", err) + } + go func() { // TCP listening goroutine - addr, err := net.ResolveTCPAddr("tcp", "localhost:0") // dynamically get a random open port from OS + conn, err := l.Accept() if err != nil { - t.Skip("unable to resolve localhost:", err) + panic(err) } - l, err := net.ListenTCP(addr.Network(), addr) + defer conn.Close() + + _, err = ioutil.ReadAll(conn) if err != nil { - t.Skip(fmt.Sprintf("unable to listen on %v: %v", addr, err)) + panic(err) } - defer l.Close() - ch0 <- l.Addr().String() - for { - conn, err := l.Accept() - if err != nil { - t.Skip("unable to accept connection:", err) - } - ch1 <- l.Addr().String() - defer conn.Close() - } - }() - go func() { // TCP client goroutine - tcpServerAddr := <-ch0 - net.Dial("tcp", tcpServerAddr) }() - tcpServerAddr := <-ch1 - tcpServerAddrIP := strings.Split(tcpServerAddr, ":")[0] - tcpServerAddrPort, err := strconv.ParseUint(strings.Split(tcpServerAddr, ":")[1], 10, 32) + conn, err := net.Dial("tcp", tcpServerAddr) if err != nil { - t.Errorf("unable to parse tcpServerAddr port: %v", err) + t.Fatalf("unable to dial %v: %v", tcpServerAddr, err) } + defer conn.Close() + c, err := p.Connections() skipIfNotImplementedErr(t, err) if err != nil { - t.Errorf("error %v", err) + t.Fatalf("error %v", err) } if len(c) == 0 { - t.Errorf("no connections found") + t.Fatal("no connections found") } found := 0 for _, connection := range c { @@ -546,7 +550,7 @@ func Test_Connections(t *testing.T) { } } if found != 2 { // two established connections, one for the server, the other for the client - t.Errorf(fmt.Sprintf("wrong connections: %+v", c)) + t.Fatalf("wrong connections: %+v", c) } } diff --git a/v3/process/process_test.go b/v3/process/process_test.go index 3d5051a..04f98ba 100644 --- a/v3/process/process_test.go +++ b/v3/process/process_test.go @@ -500,46 +500,50 @@ func Test_Parent(t *testing.T) { func Test_Connections(t *testing.T) { p := testGetProcess() - ch0 := make(chan string) - ch1 := make(chan string) + + addr, err := net.ResolveTCPAddr("tcp", "localhost:0") // dynamically get a random open port from OS + if err != nil { + t.Fatalf("unable to resolve localhost: %v", err) + } + l, err := net.ListenTCP(addr.Network(), addr) + if err != nil { + t.Fatalf("unable to listen on %v: %v", addr, err) + } + defer l.Close() + + tcpServerAddr := l.Addr().String() + tcpServerAddrIP := strings.Split(tcpServerAddr, ":")[0] + tcpServerAddrPort, err := strconv.ParseUint(strings.Split(tcpServerAddr, ":")[1], 10, 32) + if err != nil { + t.Fatalf("unable to parse tcpServerAddr port: %v", err) + } + go func() { // TCP listening goroutine - addr, err := net.ResolveTCPAddr("tcp", "localhost:0") // dynamically get a random open port from OS + conn, err := l.Accept() if err != nil { - t.Skip("unable to resolve localhost:", err) + panic(err) } - l, err := net.ListenTCP(addr.Network(), addr) + defer conn.Close() + + _, err = ioutil.ReadAll(conn) if err != nil { - t.Skip(fmt.Sprintf("unable to listen on %v: %v", addr, err)) + panic(err) } - defer l.Close() - ch0 <- l.Addr().String() - for { - conn, err := l.Accept() - if err != nil { - t.Skip("unable to accept connection:", err) - } - ch1 <- l.Addr().String() - defer conn.Close() - } - }() - go func() { // TCP client goroutine - tcpServerAddr := <-ch0 - net.Dial("tcp", tcpServerAddr) }() - tcpServerAddr := <-ch1 - tcpServerAddrIP := strings.Split(tcpServerAddr, ":")[0] - tcpServerAddrPort, err := strconv.ParseUint(strings.Split(tcpServerAddr, ":")[1], 10, 32) + conn, err := net.Dial("tcp", tcpServerAddr) if err != nil { - t.Errorf("unable to parse tcpServerAddr port: %v", err) + t.Fatalf("unable to dial %v: %v", tcpServerAddr, err) } + defer conn.Close() + c, err := p.Connections() skipIfNotImplementedErr(t, err) if err != nil { - t.Errorf("error %v", err) + t.Fatalf("error %v", err) } if len(c) == 0 { - t.Errorf("no connections found") + t.Fatal("no connections found") } found := 0 for _, connection := range c { @@ -548,7 +552,7 @@ func Test_Connections(t *testing.T) { } } if found != 2 { // two established connections, one for the server, the other for the client - t.Errorf(fmt.Sprintf("wrong connections: %+v", c)) + t.Fatalf("wrong connections: %+v", c) } } From 5ce887df8fb46bd7a533d7bf60b1be582644a0d6 Mon Sep 17 00:00:00 2001 From: Tom Barker Date: Thu, 19 Aug 2021 10:38:54 -0400 Subject: [PATCH 2/4] Make sure that Test_AllProcesses_cmdLine doesn't ignore failures. --- process/process_test.go | 46 +++++++++++++++++++++++++--------------------- v3/process/process_test.go | 46 +++++++++++++++++++++++++--------------------- 2 files changed, 50 insertions(+), 42 deletions(-) diff --git a/process/process_test.go b/process/process_test.go index 430ea50..8c59b1a 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -751,34 +751,38 @@ func Test_Process_Environ(t *testing.T) { func Test_AllProcesses_cmdLine(t *testing.T) { procs, err := Processes() - if err == nil { - for _, proc := range procs { - var exeName string - var cmdLine string - - exeName, _ = proc.Exe() - cmdLine, err = proc.Cmdline() - if err != nil { - cmdLine = "Error: " + err.Error() - } - - t.Logf("Process #%v: Name: %v / CmdLine: %v\n", proc.Pid, exeName, cmdLine) + skipIfNotImplementedErr(t, err) + if err != nil { + t.Fatalf("getting processes error %v", err) + } + for _, proc := range procs { + var exeName string + var cmdLine string + + exeName, _ = proc.Exe() + cmdLine, err = proc.Cmdline() + if err != nil { + cmdLine = "Error: " + err.Error() } + + t.Logf("Process #%v: Name: %v / CmdLine: %v\n", proc.Pid, exeName, cmdLine) } } func Test_AllProcesses_environ(t *testing.T) { procs, err := Processes() - if err == nil { - for _, proc := range procs { - exeName, _ := proc.Exe() - environ, err := proc.Environ() - if err != nil { - environ = []string{"Error: " + err.Error() } - } - - t.Logf("Process #%v: Name: %v / Environment Variables: %v\n", proc.Pid, exeName, environ) + skipIfNotImplementedErr(t, err) + if err != nil { + t.Fatalf("getting processes error %v", err) + } + for _, proc := range procs { + exeName, _ := proc.Exe() + environ, err := proc.Environ() + if err != nil { + environ = []string{"Error: " + err.Error()} } + + t.Logf("Process #%v: Name: %v / Environment Variables: %v\n", proc.Pid, exeName, environ) } } diff --git a/v3/process/process_test.go b/v3/process/process_test.go index 04f98ba..14238e7 100644 --- a/v3/process/process_test.go +++ b/v3/process/process_test.go @@ -753,34 +753,38 @@ func Test_Process_Environ(t *testing.T) { func Test_AllProcesses_cmdLine(t *testing.T) { procs, err := Processes() - if err == nil { - for _, proc := range procs { - var exeName string - var cmdLine string - - exeName, _ = proc.Exe() - cmdLine, err = proc.Cmdline() - if err != nil { - cmdLine = "Error: " + err.Error() - } - - t.Logf("Process #%v: Name: %v / CmdLine: %v\n", proc.Pid, exeName, cmdLine) + skipIfNotImplementedErr(t, err) + if err != nil { + t.Fatalf("getting processes error %v", err) + } + for _, proc := range procs { + var exeName string + var cmdLine string + + exeName, _ = proc.Exe() + cmdLine, err = proc.Cmdline() + if err != nil { + cmdLine = "Error: " + err.Error() } + + t.Logf("Process #%v: Name: %v / CmdLine: %v\n", proc.Pid, exeName, cmdLine) } } func Test_AllProcesses_environ(t *testing.T) { procs, err := Processes() - if err == nil { - for _, proc := range procs { - exeName, _ := proc.Exe() - environ, err := proc.Environ() - if err != nil { - environ = []string{"Error: " + err.Error() } - } - - t.Logf("Process #%v: Name: %v / Environment Variables: %v\n", proc.Pid, exeName, environ) + skipIfNotImplementedErr(t, err) + if err != nil { + t.Fatalf("getting processes error %v", err) + } + for _, proc := range procs { + exeName, _ := proc.Exe() + environ, err := proc.Environ() + if err != nil { + environ = []string{"Error: " + err.Error()} } + + t.Logf("Process #%v: Name: %v / Environment Variables: %v\n", proc.Pid, exeName, environ) } } From bc4661937d9b189072254f4c9e4bc7f258a751d1 Mon Sep 17 00:00:00 2001 From: Tom Barker Date: Thu, 19 Aug 2021 10:39:18 -0400 Subject: [PATCH 3/4] Minor cleanups motivated by staticcheck warnings. --- process/process_linux.go | 7 ++----- process/process_test.go | 4 ++-- v3/process/process_linux.go | 7 ++----- v3/process/process_test.go | 4 ++-- 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/process/process_linux.go b/process/process_linux.go index ead99ad..de742fb 100644 --- a/process/process_linux.go +++ b/process/process_linux.go @@ -351,7 +351,7 @@ func (p *Process) PageFaultsWithContext(ctx context.Context) (*PageFaultsStat, e func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) { pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid) if err != nil { - if pids == nil || len(pids) == 0 { + if len(pids) == 0 { return nil, ErrorNoChildren } return nil, err @@ -705,10 +705,7 @@ func (p *Process) fillFromCmdlineWithContext(ctx context.Context) (string, error return "", err } ret := strings.FieldsFunc(string(cmdline), func(r rune) bool { - if r == '\u0000' { - return true - } - return false + return r == '\u0000' }) return strings.Join(ret, " "), nil diff --git a/process/process_test.go b/process/process_test.go index 8c59b1a..d0c5dc7 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -418,14 +418,14 @@ func Test_Process_Exe(t *testing.T) { func Test_Process_CpuPercent(t *testing.T) { p := testGetProcess() - percent, err := p.Percent(0) + _, err := p.Percent(0) skipIfNotImplementedErr(t, err) if err != nil { t.Errorf("error %v", err) } duration := time.Duration(1000) * time.Microsecond time.Sleep(duration) - percent, err = p.Percent(0) + percent, err := p.Percent(0) if err != nil { t.Errorf("error %v", err) } diff --git a/v3/process/process_linux.go b/v3/process/process_linux.go index 90f61e9..a424e00 100644 --- a/v3/process/process_linux.go +++ b/v3/process/process_linux.go @@ -351,7 +351,7 @@ func (p *Process) PageFaultsWithContext(ctx context.Context) (*PageFaultsStat, e func (p *Process) ChildrenWithContext(ctx context.Context) ([]*Process, error) { pids, err := common.CallPgrepWithContext(ctx, invoke, p.Pid) if err != nil { - if pids == nil || len(pids) == 0 { + if len(pids) == 0 { return nil, ErrorNoChildren } return nil, err @@ -678,10 +678,7 @@ func (p *Process) fillFromCmdlineWithContext(ctx context.Context) (string, error return "", err } ret := strings.FieldsFunc(string(cmdline), func(r rune) bool { - if r == '\u0000' { - return true - } - return false + return r == '\u0000' }) return strings.Join(ret, " "), nil diff --git a/v3/process/process_test.go b/v3/process/process_test.go index 14238e7..df31b02 100644 --- a/v3/process/process_test.go +++ b/v3/process/process_test.go @@ -420,14 +420,14 @@ func Test_Process_Exe(t *testing.T) { func Test_Process_CpuPercent(t *testing.T) { p := testGetProcess() - percent, err := p.Percent(0) + _, err := p.Percent(0) skipIfNotImplementedErr(t, err) if err != nil { t.Errorf("error %v", err) } duration := time.Duration(1000) * time.Microsecond time.Sleep(duration) - percent, err = p.Percent(0) + percent, err := p.Percent(0) if err != nil { t.Errorf("error %v", err) } From 9248140c983b516a8fff0260093d79db10ec7a90 Mon Sep 17 00:00:00 2001 From: Tom Barker Date: Mon, 23 Aug 2021 16:30:51 -0400 Subject: [PATCH 4/4] Wait for server connection to be established before checking connections. --- process/process_test.go | 35 ++++++++++++++++++++++++++++++----- v3/process/process_test.go | 35 ++++++++++++++++++++++++++++++----- 2 files changed, 60 insertions(+), 10 deletions(-) diff --git a/process/process_test.go b/process/process_test.go index d0c5dc7..495af05 100644 --- a/process/process_test.go +++ b/process/process_test.go @@ -516,6 +516,7 @@ func Test_Connections(t *testing.T) { t.Fatalf("unable to parse tcpServerAddr port: %v", err) } + serverEstablished := make(chan struct{}) go func() { // TCP listening goroutine conn, err := l.Accept() if err != nil { @@ -523,6 +524,7 @@ func Test_Connections(t *testing.T) { } defer conn.Close() + serverEstablished <- struct{}{} _, err = ioutil.ReadAll(conn) if err != nil { panic(err) @@ -535,6 +537,10 @@ func Test_Connections(t *testing.T) { } defer conn.Close() + // Rarely the call to net.Dial returns before the server connection is + // established. Wait so that the test doesn't fail. + <-serverEstablished + c, err := p.Connections() skipIfNotImplementedErr(t, err) if err != nil { @@ -543,14 +549,33 @@ func Test_Connections(t *testing.T) { if len(c) == 0 { t.Fatal("no connections found") } - found := 0 + + serverConnections := 0 for _, connection := range c { - if connection.Status == "ESTABLISHED" && (connection.Laddr.IP == tcpServerAddrIP && connection.Laddr.Port == uint32(tcpServerAddrPort)) || (connection.Raddr.IP == tcpServerAddrIP && connection.Raddr.Port == uint32(tcpServerAddrPort)) { - found++ + if connection.Laddr.IP == tcpServerAddrIP && connection.Laddr.Port == uint32(tcpServerAddrPort) && connection.Raddr.Port != 0 { + if connection.Status != "ESTABLISHED" { + t.Fatalf("expected server connection to be ESTABLISHED, have %+v", connection) + } + serverConnections++ } } - if found != 2 { // two established connections, one for the server, the other for the client - t.Fatalf("wrong connections: %+v", c) + + clientConnections := 0 + for _, connection := range c { + if connection.Raddr.IP == tcpServerAddrIP && connection.Raddr.Port == uint32(tcpServerAddrPort) { + if connection.Status != "ESTABLISHED" { + t.Fatalf("expected client connection to be ESTABLISHED, have %+v", connection) + } + clientConnections++ + } + } + + if serverConnections != 1 { // two established connections, one for the server, the other for the client + t.Fatalf("expected 1 server connection, have %d.\nDetails: %+v", serverConnections, c) + } + + if clientConnections != 1 { // two established connections, one for the server, the other for the client + t.Fatalf("expected 1 server connection, have %d.\nDetails: %+v", clientConnections, c) } } diff --git a/v3/process/process_test.go b/v3/process/process_test.go index df31b02..a8b5645 100644 --- a/v3/process/process_test.go +++ b/v3/process/process_test.go @@ -518,6 +518,7 @@ func Test_Connections(t *testing.T) { t.Fatalf("unable to parse tcpServerAddr port: %v", err) } + serverEstablished := make(chan struct{}) go func() { // TCP listening goroutine conn, err := l.Accept() if err != nil { @@ -525,6 +526,7 @@ func Test_Connections(t *testing.T) { } defer conn.Close() + serverEstablished <- struct{}{} _, err = ioutil.ReadAll(conn) if err != nil { panic(err) @@ -537,6 +539,10 @@ func Test_Connections(t *testing.T) { } defer conn.Close() + // Rarely the call to net.Dial returns before the server connection is + // established. Wait so that the test doesn't fail. + <-serverEstablished + c, err := p.Connections() skipIfNotImplementedErr(t, err) if err != nil { @@ -545,14 +551,33 @@ func Test_Connections(t *testing.T) { if len(c) == 0 { t.Fatal("no connections found") } - found := 0 + + serverConnections := 0 for _, connection := range c { - if connection.Status == "ESTABLISHED" && (connection.Laddr.IP == tcpServerAddrIP && connection.Laddr.Port == uint32(tcpServerAddrPort)) || (connection.Raddr.IP == tcpServerAddrIP && connection.Raddr.Port == uint32(tcpServerAddrPort)) { - found++ + if connection.Laddr.IP == tcpServerAddrIP && connection.Laddr.Port == uint32(tcpServerAddrPort) && connection.Raddr.Port != 0 { + if connection.Status != "ESTABLISHED" { + t.Fatalf("expected server connection to be ESTABLISHED, have %+v", connection) + } + serverConnections++ } } - if found != 2 { // two established connections, one for the server, the other for the client - t.Fatalf("wrong connections: %+v", c) + + clientConnections := 0 + for _, connection := range c { + if connection.Raddr.IP == tcpServerAddrIP && connection.Raddr.Port == uint32(tcpServerAddrPort) { + if connection.Status != "ESTABLISHED" { + t.Fatalf("expected client connection to be ESTABLISHED, have %+v", connection) + } + clientConnections++ + } + } + + if serverConnections != 1 { // two established connections, one for the server, the other for the client + t.Fatalf("expected 1 server connection, have %d.\nDetails: %+v", serverConnections, c) + } + + if clientConnections != 1 { // two established connections, one for the server, the other for the client + t.Fatalf("expected 1 server connection, have %d.\nDetails: %+v", clientConnections, c) } }