Skip to content

Commit

Permalink
fix: add commander stopping sync mechanism (#352)
Browse files Browse the repository at this point in the history
Resolves #256

Test that generates the data race:
```go
func TestNewSupervisor(t *testing.T) {
...

	supervisor, err := NewSupervisor(&internal.NopLogger{})
	assert.NoError(t, err)

	supervisor.Shutdown()
}
```

It looks like the commander struct is started in a go routine when the `NewSupervisor` is created. The data race happens when the commander is stopped (`supervisor.Shutdown()`) while being initalized (Start).

The fix adds a synchronization mechanism to prevent the commander being started if it is already being stopped.
  • Loading branch information
rogercoll authored Feb 20, 2025
1 parent 71fdce8 commit fe44fcc
Showing 1 changed file with 10 additions and 0 deletions.
10 changes: 10 additions & 0 deletions internal/examples/supervisor/supervisor/commander/commander.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"os"
"os/exec"
"sync"
"sync/atomic"
"syscall"
"time"
Expand All @@ -24,6 +25,9 @@ type Commander struct {
doneCh chan struct{}
waitCh chan struct{}
running int64

// Process should not be started while being stopped.
startStopMutex sync.RWMutex
}

func NewCommander(logger types.Logger, cfg *config.Agent, args ...string) (*Commander, error) {
Expand All @@ -41,6 +45,9 @@ func NewCommander(logger types.Logger, cfg *config.Agent, args ...string) (*Comm
// Start the Agent and begin watching the process.
// Agent's stdout and stderr are written to a file.
func (c *Commander) Start(ctx context.Context) error {
c.startStopMutex.Lock()
defer c.startStopMutex.Unlock()

c.logger.Debugf(ctx, "Starting agent %s", c.cfg.Executable)

logFilePath := "agent.log"
Expand Down Expand Up @@ -116,6 +123,9 @@ func (c *Commander) IsRunning() bool {
// and if the process does not finish kills it forcedly by sending SIGKILL.
// Returns after the process is terminated.
func (c *Commander) Stop(ctx context.Context) error {
c.startStopMutex.Lock()
defer c.startStopMutex.Unlock()

if c.cmd == nil || c.cmd.Process == nil {
// Not started, nothing to do.
return nil
Expand Down

0 comments on commit fe44fcc

Please sign in to comment.