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

Added error handling for context.Canceled in log reading code #2268

Merged
merged 18 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -728,7 +728,7 @@ func (c *DockerContainer) startLogProduction(ctx context.Context, opts ...LogPro
since = fmt.Sprintf("%d.%09d", now.Unix(), int64(now.Nanosecond()))
goto BEGIN
}
if errors.Is(err, context.DeadlineExceeded) {
if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
// Probably safe to continue here
continue
}
Expand Down Expand Up @@ -756,7 +756,7 @@ func (c *DockerContainer) startLogProduction(ctx context.Context, opts ...LogPro
if err != nil {
// TODO: add-logger: use logger to log out this error
_, _ = fmt.Fprintf(os.Stderr, "error occurred reading log with known length %s", err.Error())
if errors.Is(err, context.DeadlineExceeded) {
if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) {
// Probably safe to continue here
continue
}
Expand Down
117 changes: 117 additions & 0 deletions logconsumer_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package testcontainers

import (
"bytes"
"context"
"errors"
"fmt"
"io"
"net/http"
"os"
"os/signal"
"strings"
"syscall"
"testing"
"time"

Expand Down Expand Up @@ -508,3 +511,117 @@ func Test_StartLogProductionStillStartsWithTooHighTimeout(t *testing.T) {

terminateContainerOnEnd(t, ctx, c)
}

func Test_MultiContainerLogConsumer_CancelledContext(t *testing.T) {
// Redirect stderr to a buffer
oldStderr := os.Stderr
r, w, _ := os.Pipe()
os.Stderr = w

// Context with cancellation functionality for handling interrupt (SIGINT) and
// termination (SIGTERM) signals.
ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you want to trap the signals here ? Could it be enough to have a cancelleable context with ctx, cancel := context.WithCancel(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JordanP :
Thank you for reviewing this PR. The reason for trapping the signal over there was to simulate an interrupt signal Ctrl+C, as that was the main motivation behind creating this PR. The objective was to prevent system hangs when multiple containers with log consumers receive an interrupt signal. As, with the latest version ( v0.28.0 ), the system hangs if someone presses Ctrl + C to stop multiple containers with log consumers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Could it be enough to have a cancelleable context with ctx, cancel := context.WithCancel(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JordanP : Thanks for your suggestion. Yes, I tried using context.WithCancel(...) instead of signal.NotifyContext(...) and it seems to have a similar effect in the test case. I have made the changes.


first := TestLogConsumer{
Msgs: []string{},
Done: make(chan bool),
Accepted: devNullAcceptorChan(),
}

containerReq1 := ContainerRequest{
FromDockerfile: FromDockerfile{
Context: "./testdata/",
Dockerfile: "echoserver.Dockerfile",
},
ExposedPorts: []string{"8080/tcp"},
WaitingFor: wait.ForLog("ready"),
LogConsumerCfg: &LogConsumerConfig{
Consumers: []LogConsumer{&first},
},
}

genericReq1 := GenericContainerRequest{
ContainerRequest: containerReq1,
Started: true,
}

c, err := GenericContainer(ctx, genericReq1)
require.NoError(t, err)

ep1, err := c.Endpoint(ctx, "http")
require.NoError(t, err)

_, err = http.Get(ep1 + "/stdout?echo=hello1")
require.NoError(t, err)

_, err = http.Get(ep1 + "/stdout?echo=there1")
require.NoError(t, err)

second := TestLogConsumer{
Msgs: []string{},
Done: make(chan bool),
Accepted: devNullAcceptorChan(),
}

containerReq2 := ContainerRequest{
FromDockerfile: FromDockerfile{
Context: "./testdata/",
Dockerfile: "echoserver.Dockerfile",
},
ExposedPorts: []string{"8080/tcp"},
WaitingFor: wait.ForLog("ready"),
LogConsumerCfg: &LogConsumerConfig{
Consumers: []LogConsumer{&second},
},
}

genericReq2 := GenericContainerRequest{
ContainerRequest: containerReq2,
Started: true,
}

c2, err := GenericContainer(ctx, genericReq2)
require.NoError(t, err)

ep2, err := c2.Endpoint(ctx, "http")
require.NoError(t, err)

_, err = http.Get(ep2 + "/stdout?echo=hello2")
require.NoError(t, err)

_, err = http.Get(ep2 + "/stdout?echo=there2")
require.NoError(t, err)

// Handling the termination of the containers
defer func() {
shutdownCtx, shutdownCancel := context.WithTimeout(
context.Background(), 60*time.Second,
)
defer shutdownCancel()
_ = c.Terminate(shutdownCtx)
_ = c2.Terminate(shutdownCtx)
}()

// Deliberately calling context cancel
cancel()

// Check the log messages
assert.Equal(t, []string{"ready\n", "echo hello1\n", "echo there1\n"}, first.Msgs)
assert.Equal(t, []string{"ready\n", "echo hello2\n", "echo there2\n"}, second.Msgs)

// Restore stderr
w.Close()
os.Stderr = oldStderr

// Read the stderr output from the buffer
var buf bytes.Buffer
_, _ = buf.ReadFrom(r)

// Check the stderr message
actual := buf.String()

// The context cancel shouldn't cause the system to throw a
// logStoppedForOutOfSyncMessage, as it hangs the system with
// the multiple containers.
assert.False(t, strings.Contains(actual, logStoppedForOutOfSyncMessage))
}