Skip to content

Commit

Permalink
log: fix crash on removing log dir
Browse files Browse the repository at this point in the history
Fix double closing the chanel on log dirs removing.
  • Loading branch information
psergee authored and oleg-jukovec committed Sep 26, 2024
1 parent 0c4094e commit 81ee2ab
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 6 deletions.
9 changes: 8 additions & 1 deletion cli/cmd/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"os/signal"
"sync"

"github.com/spf13/cobra"
"github.com/tarantool/tt/cli/cmd/internal"
Expand Down Expand Up @@ -74,10 +75,12 @@ func follow(instances []running.InstanceCtx, n int) error {
const logLinesChannelCapacity = 64
logLines := make(chan string, logLinesChannelCapacity)
tailRoutinesStarted := 0
// Wait group to wait for completion of all log reading routines to close the channel once.
var wg sync.WaitGroup
for _, inst := range instances {
if err := tail.Follow(ctx, logLines,
tail.NewLogFormatter(running.GetAppInstanceName(inst)+": ", color),
inst.Log, n); err != nil {
inst.Log, n, &wg); err != nil {
if errors.Is(err, os.ErrNotExist) {
continue
}
Expand All @@ -89,6 +92,10 @@ func follow(instances []running.InstanceCtx, n int) error {
}

if tailRoutinesStarted > 0 {
go func() {
wg.Wait()
close(logLines)
}()
return printLines(ctx, logLines)
}
return nil
Expand Down
6 changes: 4 additions & 2 deletions cli/tail/tail.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"io"
"os"
"strings"
"sync"

"github.com/apex/log"
"github.com/fatih/color"
Expand Down Expand Up @@ -131,7 +132,7 @@ func TailN(ctx context.Context, logFormatter LogFormatter, fileName string,

// Follow sends to the channel each new line from the file as it grows.
func Follow(ctx context.Context, out chan<- string, logFormatter LogFormatter, fileName string,
n int) error {
n int, wg *sync.WaitGroup) error {
file, err := os.Open(fileName)
if err != nil {
return fmt.Errorf("cannot open %q: %w", fileName, err)
Expand All @@ -157,7 +158,9 @@ func Follow(ctx context.Context, out chan<- string, logFormatter LogFormatter, f
return err
}

wg.Add(1)
go func() {
defer wg.Done()
for {
select {
case <-ctx.Done():
Expand All @@ -172,7 +175,6 @@ func Follow(ctx context.Context, out chan<- string, logFormatter LogFormatter, f
} else {
log.Errorf("The log file %q is unavailable for reading. Exiting.")
}
close(out)
return
}
out <- logFormatter(line.Text)
Expand Down
4 changes: 3 additions & 1 deletion cli/tail/tail_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"context"
"os"
"sync"
"testing"
"time"

Expand Down Expand Up @@ -299,7 +300,8 @@ func TestFollow(t *testing.T) {
defer stop()
in := make(chan string)
err = Follow(ctx, in,
func(str string) string { return str }, outFile.Name(), tt.nLines)
func(str string) string { return str }, outFile.Name(), tt.nLines,
&sync.WaitGroup{})
require.NoError(t, err)

if tt.nLines > 0 && len(tt.expectedLastLines) > 0 {
Expand Down
37 changes: 35 additions & 2 deletions test/integration/log/test_log.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ def test_log_output_default_follow_want_zero_last(tt_cmd, mock_env_dir):
assert 'app1:inst0' not in output


def test_log__dir_removed_after_follow(tt_cmd, mock_env_dir):
def test_log_dir_removed_after_follow(tt_cmd, mock_env_dir):
cmd = [tt_cmd, 'log', '-f']
process = subprocess.Popen(
cmd,
Expand All @@ -270,7 +270,40 @@ def test_log__dir_removed_after_follow(tt_cmd, mock_env_dir):
['app0:inst0: line 19', 'app1:inst2: line 19',
'app0:inst1: line 19', 'app1:inst1: line 19'])

var_dir = os.path.join(mock_env_dir, 'ie', 'app0', 'var')
var_dir = os.path.join(mock_env_dir, 'ie')
assert os.path.exists(var_dir)
shutil.rmtree(var_dir)

assert process.wait(2) == 0
assert "Failed to detect creation of" in process.stdout.read()


# There are two apps in this test: app0 and app1. After removing app0 dirs,
# tt log -f is still able to monitor the app1 log files, so there should be no issue.
def test_log_dir_partially_removed_after_follow(tt_cmd, mock_env_dir):
cmd = [tt_cmd, 'log', '-f']
process = subprocess.Popen(
cmd,
cwd=mock_env_dir,
stderr=subprocess.STDOUT,
stdout=subprocess.PIPE,
text=True,
)

wait_for_lines_in_output(process.stdout,
['app0:inst0: line 19', 'app1:inst2: line 19',
'app0:inst1: line 19', 'app1:inst1: line 19'])

# Remove one app log dir.
var_dir = os.path.join(mock_env_dir, 'ie', 'app0', 'var', 'log')
assert os.path.exists(var_dir)
shutil.rmtree(var_dir)

wait_for_lines_in_output(process.stdout, ['Failed to detect creation of'])
assert process.poll() is None # Still running.

# Remove app1 log dir.
var_dir = os.path.join(mock_env_dir, 'ie', 'app1')
assert os.path.exists(var_dir)
shutil.rmtree(var_dir)

Expand Down

0 comments on commit 81ee2ab

Please sign in to comment.