-
Notifications
You must be signed in to change notification settings - Fork 2
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
Restore signals, add pre-stop sleep #88
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ func withSignals(ctx context.Context, chSignals chan os.Signal, sig ...os.Signal | |
|
||
// function invoke cancel once a signal arrived OR parent context is done: | ||
go func() { | ||
defer signal.Stop(chSignals) | ||
defer cancel() | ||
|
||
select { | ||
|
@@ -140,6 +141,10 @@ func (t *Terminator) waitShutdown(appCtx context.Context) { | |
go func(f Hook) { | ||
defer runWg.Done() | ||
|
||
if f.preStop > 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it a good idea to introduce global pre-stop sleep instead of per-hook one to resolve this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO yes, what might be the point of having that for each hook? What are the use-cases ? |
||
time.Sleep(f.preStop) | ||
} | ||
|
||
ctx, cancel := context.WithTimeout(context.Background(), f.timeout) | ||
defer cancel() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,9 +3,11 @@ package graterm | |
import ( | ||
"context" | ||
"errors" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
"log" | ||
"os" | ||
"os/exec" | ||
"os/signal" | ||
"runtime" | ||
"sync" | ||
|
@@ -277,6 +279,39 @@ func TestTerminator_Wait(t *testing.T) { | |
} | ||
} | ||
|
||
func TestTerminator_restores_signals(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test is more like a This timing-specific test relying on the scheduler are tricky to implement right, consider mocking time adding assertions that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test actually is not timing-specific because it tests signal default handler restoration. I'll try to test without sleeps between signals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's scheduler timing-specific because you rely on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed sleeps before sending SIGINT's locally. Test doesn't fail. So it shouldn't be flaky at all because we're waiting for process termination and checking for specific flags. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xakep666 now it looks better, thank you. What about having a separate integration test suite in a separate file ? So we could leverage integration testing with this approach for other cases as well and not mix it with unit tests There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good idea but |
||
if os.Getenv("HANG_ON_STOP") == "1" { | ||
terminator, ctx := NewWithSignals(context.Background(), syscall.SIGINT, syscall.SIGTERM) | ||
require.NotNil(t, ctx) | ||
|
||
terminator.WithOrder(0).Register(5*time.Second, func(ctx context.Context) { | ||
select {} | ||
}) | ||
|
||
_ = terminator.Wait(ctx, time.Minute) | ||
return | ||
} | ||
|
||
cmd := exec.Command(os.Args[0], "-test.run=TestTerminator_restores_signal") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not a good approach to introduce a recursion in tests, pls see the comment above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Subprocess-based testing is a widely used technique There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hm, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Distroless" images from Google doesn't contain coreutils. $ docker run --rm -it gcr.io/distroless/base sleep 1
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: exec: "sleep": executable file not found in $PATH: unknown. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking generally about this approach I believe it makes sense when you do integration testing of your CLI application, so it's easy an interesting approach to bind the test case to the CLI program execution, as in the given article they do in Hashicorp For this particular case testing a package providing a utility structure it's an overkill IMO |
||
cmd.Env = append(cmd.Env, "HANG_ON_STOP=1") | ||
require.NoError(t, cmd.Start()) | ||
|
||
time.Sleep(100 * time.Millisecond) | ||
require.NoError(t, cmd.Process.Signal(syscall.SIGINT)) | ||
|
||
time.Sleep(100 * time.Millisecond) | ||
require.NoError(t, cmd.Process.Signal(syscall.SIGINT)) | ||
|
||
var exitErr *exec.ExitError | ||
require.ErrorAs(t, cmd.Wait(), &exitErr) | ||
|
||
ws, ok := exitErr.Sys().(syscall.WaitStatus) | ||
require.Truef(t, ok, "exit error is not syscall.WaitStatus, but %T", exitErr.Sys()) | ||
|
||
assert.True(t, ws.Signaled(), "process stopped not by signal") | ||
assert.Equal(t, syscall.SIGINT, ws.Signal(), "process stopped not by SIGINT") | ||
} | ||
|
||
func Test_withSignals(t *testing.T) { | ||
t.Run("termination_on_SIGHUP", func(t *testing.T) { | ||
rootCtx, cancel := context.WithCancel(context.Background()) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍