Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Race condition in RunCmdWithOutputParser #41

Merged
merged 5 commits into from
Nov 30, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 2 additions & 2 deletions .github/workflows/analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
- name: Install Go
uses: actions/setup-go@v3
with:
go-version: 1.19.x
go-version: 1.20.x
- name: Static Code Analysis
uses: dominikh/staticcheck-action@v1
with:
Expand All @@ -23,7 +23,7 @@ jobs:
- name: Install Go
uses: actions/setup-go@v3
with:
go-version: 1.19.x
go-version: 1.20.x
- name: Install gosec
run: curl -sfL https://raw.githubusercontent.com/securego/gosec/master/install.sh | sh -s -- -b $(go env GOPATH)/bin
- name: Run gosec
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/frogbot-fix-go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ jobs:
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: 1.19.x
go-version: 1.20.x

- uses: jfrog/frogbot@v2
env:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/frogbot-scan-pr-go.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ jobs:
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: 1.19.x
go-version: 1.20.x

- uses: jfrog/frogbot@v2
env:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ jobs:
- name: Install Go
uses: actions/setup-go@v3
with:
go-version: 1.19.x
go-version: 1.20.x

- name: Go Cache
uses: actions/cache@v3
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module github.com/jfrog/gofrog

go 1.19
go 1.20

require (
github.com/mholt/archiver/v3 v3.5.1
Expand Down
130 changes: 99 additions & 31 deletions io/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@ package io

import (
"bufio"
"bytes"
"errors"
"fmt"
"io"
"os"
"os/exec"
"regexp"
"strings"
"sync"
)

Expand Down Expand Up @@ -87,54 +89,34 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
return
}
errChan := make(chan error)
stdoutBuilder := strings.Builder{}
wg.Add(1)
go func() {
defer wg.Done()
for stdoutReader.Scan() {
line := stdoutReader.Text()
for _, regExp := range regExpStruct {
matched := regExp.RegExp.Match([]byte(line))
if matched {
regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line)
regExp.Line = line
line, err = regExp.ExecFunc(regExp)
if err != nil {
errChan <- err
}
}
}
line, _ := processLine(regExpStruct, stdoutReader.Text(), errChan)
if prompt {
fmt.Fprintf(os.Stderr, line+"\n")
}
stdOut += line + "\n"
stdoutBuilder.WriteString(line)
stdoutBuilder.WriteRune('\n')
}
wg.Done()
}()
stderrBuilder := strings.Builder{}
wg.Add(1)
go func() {
defer wg.Done()
for stderrReader.Scan() {
line := stderrReader.Text()
var scannerError error
for _, regExp := range regExpStruct {
matched := regExp.RegExp.Match([]byte(line))
if matched {
regExp.MatchedResults = regExp.RegExp.FindStringSubmatch(line)
regExp.Line = line
line, scannerError = regExp.ExecFunc(regExp)
if scannerError != nil {
errChan <- scannerError
break
}
}
}
line, hasError := processLine(regExpStruct, stderrReader.Text(), errChan)
if prompt {
fmt.Fprintf(os.Stderr, line+"\n")
}
errorOut += line + "\n"
if scannerError != nil {
stderrBuilder.WriteString(line)
stderrBuilder.WriteRune('\n')
if hasError {
break
}
}
wg.Done()
}()

go func() {
Expand All @@ -145,6 +127,8 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
for err = range errChan {
return
}
stdOut = stdoutBuilder.String()
errorOut = stderrBuilder.String()

err = cmd.Wait()
if err != nil {
Expand All @@ -158,6 +142,34 @@ func RunCmdWithOutputParser(config CmdConfig, prompt bool, regExpStruct ...*CmdO
return
}

// Run all of the input regExpStruct array on the input stdout or stderr line.
// If an error occurred, add it to the error channel.
// regExpStruct - Array of command output patterns to process the line
// line - string line from stdout or stderr
// errChan - if an error occurred, add it to this channel
func processLine(regExpStruct []*CmdOutputPattern, line string, errChan chan error) (processedLine string, hasError bool) {
var err error
processedLine = line
for _, regExp := range regExpStruct {
matched := regExp.RegExp.MatchString(processedLine)
if matched {
results := CmdOutputPattern{
RegExp: regExp.RegExp,
MatchedResults: regExp.RegExp.FindStringSubmatch(processedLine),
Line: processedLine,
ExecFunc: regExp.ExecFunc,
}
processedLine, err = regExp.ExecFunc(&results)
if err != nil {
errChan <- err
hasError = true
break
}
}
}
yahavi marked this conversation as resolved.
Show resolved Hide resolved
return
}

// Create command stdout and stderr readers.
// The returned readers are automatically closed after the running command exit and shouldn't be closed explicitly.
// cmd - The command to execute
Expand Down Expand Up @@ -192,3 +204,59 @@ type CmdOutputPattern struct {
Line string
ExecFunc func(pattern *CmdOutputPattern) (string, error)
}

type Command struct {
Executable string
CmdName string
CmdArgs []string
Dir string
StrWriter io.WriteCloser
ErrWriter io.WriteCloser
}

func NewCommand(executable, cmdName string, cmdArgs []string) *Command {
return &Command{Executable: executable, CmdName: cmdName, CmdArgs: cmdArgs}
}

func (config *Command) RunWithOutput() (data []byte, err error) {
cmd := config.GetCmd()
var stdout bytes.Buffer
var stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr
err = cmd.Run()
if err != nil {
return nil, fmt.Errorf("failed running command: '%s %s' with error: %s - %s",
cmd.Dir,
strings.Join(cmd.Args, " "),
err.Error(),
stderr.String(),
)
}
return stdout.Bytes(), nil
}

func (config *Command) GetCmd() (cmd *exec.Cmd) {
var cmdStr []string
if config.CmdName != "" {
cmdStr = append(cmdStr, config.CmdName)
}
if config.CmdArgs != nil && len(config.CmdArgs) > 0 {
cmdStr = append(cmdStr, config.CmdArgs...)
}
cmd = exec.Command(config.Executable, cmdStr...)
cmd.Dir = config.Dir
return
}

func (config *Command) GetEnv() map[string]string {
return map[string]string{}
}

func (config *Command) GetStdWriter() io.WriteCloser {
return config.StrWriter
}

func (config *Command) GetErrWriter() io.WriteCloser {
return config.ErrWriter
}
97 changes: 97 additions & 0 deletions io/cmd_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package io

import (
"errors"
"regexp"
"testing"

"github.com/stretchr/testify/assert"
)

var matchAllRegexp = regexp.MustCompile(".*")
var errParsing = errors.New("parsing error")

func TestRunCmdWithOutputParser(t *testing.T) {
config := NewCommand("go", "", []string{"version"})
stdout, stderr, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil },
})
assert.NoError(t, err)
yahavi marked this conversation as resolved.
Show resolved Hide resolved
assert.True(t, exitOk)
assert.Contains(t, stdout, "go version")
assert.Empty(t, stderr)
}

func TestRunCmdWithOutputParserError(t *testing.T) {
config := NewCommand("go", "", []string{"version"})
_, _, exitOk, err := RunCmdWithOutputParser(config, false, &CmdOutputPattern{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, errParsing },
})
assert.ErrorContains(t, err, "parsing error")
assert.False(t, exitOk)
}

var processLineCases = []struct {
cmdOutputPatterns []*CmdOutputPattern
line string
expectedOutput string
expectError bool
}{
{[]*CmdOutputPattern{}, "", "", false},

{[]*CmdOutputPattern{{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line, nil },
}}, "hello", "hello", false},

{[]*CmdOutputPattern{{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line[1:], nil },
}}, "hello", "ello", false},

{[]*CmdOutputPattern{
{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "l", nil },
},
{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil },
},
}, "hel", "hello", false},

{[]*CmdOutputPattern{
{
RegExp: regexp.MustCompile("doesn't match"),
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "aaaaaa", nil },
},
{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return pattern.Line + "o", nil },
},
}, "hell", "hello", false},

{[]*CmdOutputPattern{{
RegExp: matchAllRegexp,
ExecFunc: func(pattern *CmdOutputPattern) (string, error) { return "", errParsing },
}}, "hello", "", true},
}

func TestProcessLine(t *testing.T) {
for _, testCase := range processLineCases {
t.Run("", func(t *testing.T) {
yahavi marked this conversation as resolved.
Show resolved Hide resolved
errChan := make(chan error, 1)
defer close(errChan)
processedLine, hasErrors := processLine(testCase.cmdOutputPatterns, testCase.line, errChan)
if testCase.expectError {
assert.True(t, hasErrors)
assert.ErrorIs(t, errParsing, <-errChan)
} else {
assert.False(t, hasErrors)
assert.Equal(t, testCase.expectedOutput, processedLine)
}
})
}
}
Loading