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

Fix race condition around supervisor's Commander #291

Closed
Closed
Changes from 1 commit
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
Prev Previous commit
Add test for verifying proper shutdown
Signed-off-by: Paschalis Tsilias <paschalis.tsilias@grafana.com>
tpaschalis committed Aug 29, 2024
commit f135e3b0ef6c4282388497bb9df315973f7e6120
33 changes: 32 additions & 1 deletion internal/examples/supervisor/supervisor/supervisor_test.go
Original file line number Diff line number Diff line change
@@ -4,7 +4,8 @@ import (
"fmt"
"os"
"testing"

"time"

"github.com/stretchr/testify/assert"

"github.com/open-telemetry/opamp-go/internal"
@@ -62,3 +63,33 @@ agent:

supervisor.Shutdown()
}

func TestShutdownRaceCondition(t *testing.T) {
tmpDir := changeCurrentDir(t)
os.WriteFile("supervisor.yaml", []byte(fmt.Sprintf(`
server:
endpoint: ws://127.0.0.1:4320/v1/opamp
agent:
executable: %s/dummy_agent.sh`, tmpDir)), 0644)

os.WriteFile("dummy_agent.sh", []byte("#!/bin/sh\nsleep 9999\n"), 0755)

startOpampServer(t)

// There's no great way to ensure Shutdown gets called before Start.
// The DelayLogger ensures some delay before the goroutine gets started.
var supervisor *Supervisor
var err error
supervisor, err = NewSupervisor(&internal.DelayLogger{})
supervisor.Shutdown()
supervisor.hasNewConfig <- struct{}{}

assert.NoError(t, err)

// The Shutdown method has been called before the runAgentProcess goroutine
// gets started and has a chance to load a new process. Make sure no PID
// has been launched.
assert.Never(t, func() bool {
return supervisor.commander.Pid() != 0
}, 2*time.Second, 10*time.Millisecond)
}
10 changes: 10 additions & 0 deletions internal/noplogger.go
Original file line number Diff line number Diff line change
@@ -2,6 +2,7 @@

import (
"context"
"time"

"github.com/open-telemetry/opamp-go/client/types"
)
@@ -12,3 +13,12 @@

func (l *NopLogger) Debugf(ctx context.Context, format string, v ...interface{}) {}
func (l *NopLogger) Errorf(ctx context.Context, format string, v ...interface{}) {}

type DelayLogger struct{}

func (l *DelayLogger) Debugf(ctx context.Context, format string, v ...interface{}) {
time.Sleep(10 * time.Millisecond)

Check warning on line 20 in internal/noplogger.go

Codecov / codecov/patch

internal/noplogger.go#L19-L20

Added lines #L19 - L20 were not covered by tests
}
func (l *DelayLogger) Errorf(ctx context.Context, format string, v ...interface{}) {
time.Sleep(10 * time.Millisecond)

Check warning on line 23 in internal/noplogger.go

Codecov / codecov/patch

internal/noplogger.go#L22-L23

Added lines #L22 - L23 were not covered by tests
}