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.

Sign up for GitHub

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

ctrl+c handling in core/commandline #2788

Merged
merged 2 commits into from
Jun 29, 2024
Merged

ctrl+c handling in core/commandline #2788

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e37c038
WIP:ctrl+c handling + test for commandline
kathiouchka Jun 25, 2024
20b59de
removing faulty tests from PR
kathiouchka Jun 25, 2024
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
21 changes: 12 additions & 9 deletions core/commandline/shell.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,16 @@ package commandline
import (
"context"
"fmt"
"github.com/c-bata/go-prompt"
"github.com/c-bata/go-prompt/completer"
"github.com/pkg/errors"
"github.com/urfave/cli/v2"
"os"
"os/signal"
"os/user"
"strings"
"syscall"

"github.com/c-bata/go-prompt"
"github.com/c-bata/go-prompt/completer"
"github.com/pkg/errors"
"github.com/urfave/cli/v2"
)

const shellCommandName = "shell"
Expand Down Expand Up @@ -50,14 +51,16 @@ func GenerateShellCommand(shellCommands []*cli.Command) *cli.Command {
}
}

// warn user about sigterms
sigs := make(chan os.Signal)
ctx, cancel := context.WithCancel(c.Context)
go func() {
for range sigs {
fmt.Printf("\n(type \"%s\", \"%s\" or \"%s\" to exit)\n\n >", quitCommand, exitCommand, quitCommandShort)
for sig := range sigs {
if sig == syscall.SIGINT || sig == syscall.SIGTERM {
cancel()
fmt.Printf("\n(type \"%s\", \"%s\" or \"%s\" to exit)\n\n >", quitCommand, exitCommand, quitCommandShort)
}
Comment on lines +55 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

Enhance signal handling and context management in GenerateShellCommand.

  • The integration of context.WithCancel and the setup of signal handling with signal.Notify for SIGINT and SIGTERM are well-implemented. This ensures that the command can gracefully handle termination signals.
  • Consider adding more detailed logging or feedback mechanisms to inform the user about the state changes when signals are received. This could improve user experience and debuggability.
  • Ensure that the signal handling does not interfere with other parts of the application, especially in a multi-threaded environment.

}
}()
//nolint: govet
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
defer func() {
signal.Stop(sigs)
Expand All @@ -69,7 +72,7 @@ func GenerateShellCommand(shellCommands []*cli.Command) *cli.Command {
return nil
}

interactive := newInteractiveClient(c.Context, capturedCommands, console)
interactive := newInteractiveClient(ctx, capturedCommands, console)
for {
p := prompt.New(
interactive.executor,
Expand Down
63 changes: 63 additions & 0 deletions core/commandline/shell_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package commandline

Check failure on line 1 in core/commandline/shell_test.go

View workflow job for this annotation

GitHub Actions / Lint (core) 

package should be `commandline_test` instead of `commandline` (testpackage)

import (
"context"
"fmt"
"os"
"os/signal"
"syscall"
"testing"
"time"

"github.com/urfave/cli/v2"
)

func TestSignalHandling(t *testing.T) {
shellCommands := []*cli.Command{
{
Name: "test",
Usage: "test command",
Action: func(c *cli.Context) error {
return nil
},
},
}

app := &cli.App{
Commands: []*cli.Command{
GenerateShellCommand(shellCommands),
},
}

// Set up a context that we can cancel
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

go func() {
time.Sleep(1 * time.Second)
p, err := os.FindProcess(os.Getpid())
if err != nil {
t.Errorf("Failed to find process: %v", err)
return
}
if err := p.Signal(syscall.SIGINT); err != nil {
t.Errorf("Failed to send SIGINT: %v", err)
}
}()

err := app.RunContext(ctx, []string{"app", "shell"})
if err != nil {
t.Fatalf("app.RunContext failed: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Ensure proper cleanup and context handling in TestSignalHandling.

  • The TestSignalHandling function correctly sets up a context and sends a SIGINT signal to the process.
  • The test includes error handling for the signal sending process.
  • The rg output indicates that SIGINT handling is implemented in shell.go, but there are no explicit assertions or logs in shell_test.go to verify that the application respects the SIGINT signal in terms of state changes or cleanup.

To improve the test, consider adding assertions or logs to confirm that the application performs the expected cleanup when receiving a SIGINT.

Analysis chain

Ensure proper cleanup and context handling in TestSignalHandling.

  • The use of defer cancel() after creating a context with context.WithCancel is a good practice to ensure resources are properly released.
  • Sending a SIGINT to the process using os.FindProcess and p.Signal is correctly implemented. However, consider handling potential errors more robustly or ensuring the test fails gracefully if the setup fails.
  • The test checks the application's response to SIGINT, but it could be improved by verifying that the application indeed respects the SIGINT in terms of state changes or cleanup.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the SIGINT handling correctly stops the application and cleans up resources.

# Test: Search for logs or state changes indicating proper SIGINT handling.
rg --type go "SIGINT"

Length of output: 517

}

func TestMain(m *testing.M) {
// Call signal.Notify so that the test process does not exit on SIGINT
sigs := make(chan os.Signal, 1)
signal.Notify(sigs, syscall.SIGINT)

go func() {
sig := <-sigs
fmt.Printf("Received signal: %v", sig)
}()
}
Loading