fifo: Close connections and cleanup lock handling

hashicorp · Jul 1, 2019 · 8148466 · 8148466
1 parent 2e5aba9
commit 8148466
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 27 deletions.
diff --git a/client/lib/fifo/fifo_windows.go b/client/lib/fifo/fifo_windows.go
@@ -23,7 +23,6 @@ type winFIFO struct {
 
 func (f *winFIFO) Read(p []byte) (n int, err error) {
 	f.connLock.Lock()
-	defer f.connLock.Unlock()
 	if f.conn == nil {
 		c, err := f.listener.Accept()
 		if err != nil {
@@ -32,6 +31,7 @@ func (f *winFIFO) Read(p []byte) (n int, err error) {
 
 		f.conn = c
 	}
+	f.connLock.Unlock()
 
 	// If the connection is closed then we need to close the listener
 	// to emulate unix fifo behavior
@@ -44,7 +44,6 @@ func (f *winFIFO) Read(p []byte) (n int, err error) {
 
 func (f *winFIFO) Write(p []byte) (n int, err error) {
 	f.connLock.Lock()
-	defer f.connLock.Unlock()
 	if f.conn == nil {
 		c, err := f.listener.Accept()
 		if err != nil {
@@ -53,18 +52,25 @@ func (f *winFIFO) Write(p []byte) (n int, err error) {
 
 		f.conn = c
 	}
+	f.connLock.Unlock()
 
 	// If the connection is closed then we need to close the listener
 	// to emulate unix fifo behavior
 	n, err = f.conn.Write(p)
 	if err == io.EOF {
+		f.conn.Close()
 		f.listener.Close()
 	}
 	return n, err
 
 }
 
 func (f *winFIFO) Close() error {
+	f.connLock.Lock()
+	if f.conn != nil {
+		f.conn.Close()
+	}
+	f.connLock.Unlock()
 	return f.listener.Close()
 }
 

diff --git a/client/logmon/logmon_test.go b/client/logmon/logmon_test.go
@@ -17,10 +17,6 @@ import (
 
 func TestLogmon_Start_rotate(t *testing.T) {
 	require := require.New(t)
-
-	stdoutLog := "stdout"
-	stderrLog := "stderr"
-
 	var stdoutFifoPath, stderrFifoPath string
 
 	dir, err := ioutil.TempDir("", "nomadtest")
@@ -37,9 +33,9 @@ func TestLogmon_Start_rotate(t *testing.T) {
 
 	cfg := &LogConfig{
 		LogDir:        dir,
-		StdoutLogFile: stdoutLog,
+		StdoutLogFile: "stdout",
 		StdoutFifo:    stdoutFifoPath,
-		StderrLogFile: stderrLog,
+		StderrLogFile: "stderr",
 		StderrFifo:    stderrFifoPath,
 		MaxFiles:      2,
 		MaxFileSizeMB: 1,
@@ -78,17 +74,13 @@ func TestLogmon_Start_rotate(t *testing.T) {
 }
 
 // asserts that calling Start twice restarts the log rotator and that any logs
-// published while the listener was unavailable are recieved.
+// published while the listener was unavailable are received.
 func TestLogmon_Start_restart_flusheslogs(t *testing.T) {
 	if runtime.GOOS == "windows" {
 		t.Skip("windows does not support pushing data to a pipe with no servers")
 	}
 
 	require := require.New(t)
-
-	stdoutLog := "stdout"
-	stderrLog := "stderr"
-
 	var stdoutFifoPath, stderrFifoPath string
 
 	dir, err := ioutil.TempDir("", "nomadtest")
@@ -105,9 +97,9 @@ func TestLogmon_Start_restart_flusheslogs(t *testing.T) {
 
 	cfg := &LogConfig{
 		LogDir:        dir,
-		StdoutLogFile: stdoutLog,
+		StdoutLogFile: "stdout",
 		StdoutFifo:    stdoutFifoPath,
-		StderrLogFile: stderrLog,
+		StderrLogFile: "stderr",
 		StderrFifo:    stderrFifoPath,
 		MaxFiles:      2,
 		MaxFileSizeMB: 1,
@@ -148,11 +140,6 @@ func TestLogmon_Start_restart_flusheslogs(t *testing.T) {
 		require.NoError(err)
 	})
 
-	require.NoError(lm.Stop())
-
-	// Start logmon again and assert that it appended to the file
-	require.NoError(lm.Start(cfg))
-
 	stdout, err = fifo.OpenWriter(stdoutFifoPath)
 	require.NoError(err)
 	stderr, err = fifo.OpenWriter(stderrFifoPath)
@@ -171,6 +158,14 @@ func TestLogmon_Start_restart_flusheslogs(t *testing.T) {
 		require.NoError(err)
 	})
 
+	// Start logmon again and assert that it appended to the file
+	require.NoError(lm.Start(cfg))
+
+	stdout, err = fifo.OpenWriter(stdoutFifoPath)
+	require.NoError(err)
+	stderr, err = fifo.OpenWriter(stderrFifoPath)
+	require.NoError(err)
+
 	_, err = stdout.Write([]byte("st\n"))
 	require.NoError(err)
 	testutil.WaitForResult(func() (bool, error) {
@@ -189,10 +184,6 @@ func TestLogmon_Start_restart_flusheslogs(t *testing.T) {
 // asserts that calling Start twice restarts the log rotator
 func TestLogmon_Start_restart(t *testing.T) {
 	require := require.New(t)
-
-	stdoutLog := "stdout"
-	stderrLog := "stderr"
-
 	var stdoutFifoPath, stderrFifoPath string
 
 	dir, err := ioutil.TempDir("", "nomadtest")
@@ -209,9 +200,9 @@ func TestLogmon_Start_restart(t *testing.T) {
 
 	cfg := &LogConfig{
 		LogDir:        dir,
-		StdoutLogFile: stdoutLog,
+		StdoutLogFile: "stdout",
 		StdoutFifo:    stdoutFifoPath,
-		StderrLogFile: stderrLog,
+		StderrLogFile: "stderr",
 		StderrFifo:    stderrFifoPath,
 		MaxFiles:      2,
 		MaxFileSizeMB: 1,
@@ -252,7 +243,7 @@ func TestLogmon_Start_restart(t *testing.T) {
 		require.NoError(err)
 	})
 
-	// Start logmon again and assert that it can recieve logs again
+	// Start logmon again and assert that it can receive logs again
 	require.NoError(lm.Start(cfg))
 
 	stdout, err = fifo.OpenWriter(stdoutFifoPath)