Verify Reaper state to create new or return existing instance (#782)

* Verify Reaper state to create new or return existing instance

Fix linting errors

* Play nice with other integration tests

* Rename reaperSingleton to reaperInstance

* Don't skip for short tests, always run

docker.go

@@ -984,7 +984,7 @@ func (p *DockerProvider) CreateContainer(ctx context.Context, req ContainerReque
 	// the reaper does not need to start a reaper for itself
 	isReaperContainer := strings.EqualFold(req.Image, reaperImage(reaperOpts.ImageName))
 	if !req.SkipReaper && !isReaperContainer {
-		r, err := newReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
+		r, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
 		if err != nil {
 			return nil, fmt.Errorf("%w: creating reaper failed", err)
 		}
@@ -1201,7 +1201,7 @@ func (p *DockerProvider) ReuseOrCreateContainer(ctx context.Context, req Contain
 	sessionID := sessionID()
 	var termSignal chan bool
 	if !req.SkipReaper {
-		r, err := newReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
+		r, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
 		if err != nil {
 			return nil, fmt.Errorf("%w: creating reaper failed", err)
 		}
@@ -1356,7 +1356,7 @@ func (p *DockerProvider) CreateNetwork(ctx context.Context, req NetworkRequest)
 	var termSignal chan bool
 	if !req.SkipReaper {
 		sessionID := sessionID()
-		r, err := newReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
+		r, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, p.host), sessionID.String(), p, req.ReaperOptions...)
 		if err != nil {
 			return nil, fmt.Errorf("%w: creating network reaper failed", err)
 		}

reaper.go

@@ -24,8 +24,8 @@ const (
 )
 
 var (
-	reaper *Reaper // We would like to create reaper only once
-	mutex  sync.Mutex
+	reaperInstance *Reaper // We would like to create reaper only once
+	mutex          sync.Mutex
 )
 
 // ReaperProvider represents a provider for the reaper to run itself with
@@ -38,22 +38,39 @@ type ReaperProvider interface {
 // NewReaper creates a Reaper with a sessionID to identify containers and a provider to use
 // Deprecated: it's not possible to create a reaper anymore.
 func NewReaper(ctx context.Context, sessionID string, provider ReaperProvider, reaperImageName string) (*Reaper, error) {
-	return newReaper(ctx, sessionID, provider, WithImageName(reaperImageName))
+	return reuseOrCreateReaper(ctx, sessionID, provider, WithImageName(reaperImageName))
 }
 
-// newReaper creates a Reaper with a sessionID to identify containers and a provider to use
-func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) {
+// reuseOrCreateReaper returns an existing Reaper instance if it exists and is running. Otherwise, a new Reaper instance
+// will be created with a sessionID to identify containers and a provider to use
+func reuseOrCreateReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) {
 	mutex.Lock()
 	defer mutex.Unlock()
-	// If reaper already exists re-use it
-	if reaper != nil {
-		return reaper, nil
+	// If reaper already exists and healthy, re-use it
+	if reaperInstance != nil {
+		// Verify this instance is still running by checking state.
+		// Can't use Container.IsRunning because the bool is not updated when Reaper is terminated
+		state, err := reaperInstance.container.State(ctx)
+		if err == nil && state.Running {
+			return reaperInstance, nil
+		}
 	}
 
+	r, err := newReaper(ctx, sessionID, provider, opts...)
+	if err != nil {
+		return nil, err
+	}
+
+	reaperInstance = r
+	return reaperInstance, nil
+}
+
+// newReaper creates a Reaper with a sessionID to identify containers and a provider to use
+// Should only be used internally and instead use reuseOrCreateReaper to prefer reusing an existing Reaper instance
+func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, opts ...ContainerOption) (*Reaper, error) {
 	dockerHost := testcontainersdocker.ExtractDockerHost(ctx)
 
-	// Otherwise create a new one
-	reaper = &Reaper{
+	reaper := &Reaper{
 		Provider:  provider,
 		SessionID: sessionID,
 	}
@@ -104,6 +121,7 @@ func newReaper(ctx context.Context, sessionID string, provider ReaperProvider, o
 	if err != nil {
 		return nil, err
 	}
+	reaper.container = c
 
 	endpoint, err := c.PortEndpoint(ctx, "8080", "")
 	if err != nil {
@@ -119,6 +137,7 @@ type Reaper struct {
 	Provider  ReaperProvider
 	SessionID string
 	Endpoint  string
+	container Container
 }
 
 // Connect runs a goroutine which can be terminated by sending true into the returned channel

reaper_test.go

@@ -57,8 +57,6 @@ func createContainerRequest(customize func(ContainerRequest) ContainerRequest) C
 }
 
 func Test_NewReaper(t *testing.T) {
-	defer func() { reaper = nil }()
-
 	type cases struct {
 		name   string
 		req    ContainerRequest
@@ -105,8 +103,6 @@ func Test_NewReaper(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			// make sure we re-initialize the singleton
-			reaper = nil
 			provider := &mockReaperProvider{
 				config: test.config,
 			}
@@ -125,8 +121,6 @@ func Test_NewReaper(t *testing.T) {
 }
 
 func Test_ReaperForNetwork(t *testing.T) {
-	defer func() { reaper = nil }()
-
 	ctx := context.Background()
 
 	networkName := "test-network-with-custom-reaper"
@@ -153,3 +147,28 @@ func Test_ReaperForNetwork(t *testing.T) {
 	assert.Equal(t, "reaperImage", provider.req.Image)
 	assert.Equal(t, "reaperImage", provider.req.ReaperImage)
 }
+
+func Test_ReaperReusedIfHealthy(t *testing.T) {
+	SkipIfProviderIsNotHealthy(&testing.T{})
+
+	ctx := context.Background()
+	// As other integration tests run with the (shared) Reaper as well, re-use the instance to not interrupt other tests
+	wasReaperRunning := reaperInstance != nil
+
+	provider, _ := ProviderDocker.GetProvider()
+	reaper, err := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider)
+	assert.NoError(t, err, "creating the Reaper should not error")
+
+	reaperReused, _ := reuseOrCreateReaper(context.WithValue(ctx, testcontainersdocker.DockerHostContextKey, provider.(*DockerProvider).host), "sessionId", provider)
+	assert.Same(t, reaper, reaperReused, "expecting the same reaper instance is returned if running and healthy")
+
+	terminate, err := reaper.Connect()
+	defer func(term chan bool) {
+		term <- true
+	}(terminate)
+	assert.NoError(t, err, "connecting to Reaper should be successful")
+
+	if !wasReaperRunning {
+		terminateContainerOnEnd(t, ctx, reaper.container)
+	}
+}