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

Handle SIGINT to abort hanging queries in ValidateBuilderExists #2214

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
51 changes: 37 additions & 14 deletions internal/commands/config_default_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ package commands

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

"github.com/pkg/errors"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -48,7 +52,7 @@ func ConfigDefaultBuilder(logger logging.Logger, cfg config.Config, cfgPath stri
return nil
default:
imageName := args[0]
if err := validateBuilderExists(logger, imageName, client); err != nil {
if err := ValidateBuilderExists(logger, imageName, client); err != nil {
return errors.Wrapf(err, "validating that builder %s exists", style.Symbol(imageName))
}

Expand All @@ -68,24 +72,43 @@ func ConfigDefaultBuilder(logger logging.Logger, cfg config.Config, cfgPath stri
return cmd
}

func validateBuilderExists(logger logging.Logger, imageName string, client PackClient) error {
logger.Debug("Verifying local image...")
info, err := client.InspectBuilder(imageName, true)
if err != nil {
return err
}
func ValidateBuilderExists(logger logging.Logger, imageName string, client PackClient) error {
sigChan := make(chan os.Signal, 1)
signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM)
Comment on lines +76 to +77
Copy link
Member

Choose a reason for hiding this comment

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

We'll probably want a separate implementation of this for linux/darwin and windows. This can be accomplished with separate files, such as signal_unix.go and signal_windows.go, and build tags as shown here.

Copy link
Member

Choose a reason for hiding this comment

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

+1 with this comment


resultChan := make(chan error, 1)

if info == nil {
logger.Debug("Verifying remote image...")
info, err := client.InspectBuilder(imageName, false)
go func() {
logger.Debug("Verifying local image...")
info, err := client.InspectBuilder(imageName, true)
if err != nil {
return errors.Wrapf(err, "failed to inspect remote image %s", style.Symbol(imageName))
resultChan <- err
return
}

if info == nil {
return fmt.Errorf("builder %s not found", style.Symbol(imageName))
logger.Debug("Verifying remote image...")
info, err = client.InspectBuilder(imageName, false)
if err != nil {
resultChan <- errors.Wrapf(err, "failed to inspect remote image %s", style.Symbol(imageName))
return
}

if info == nil {
resultChan <- fmt.Errorf("builder %s not found", style.Symbol(imageName))
return
}
}
}

return nil
resultChan <- nil
}()

select {
case err := <-resultChan:
return err
case <-sigChan:
return fmt.Errorf("operation aborted")
case <-time.After(30 * time.Second):
return fmt.Errorf("operation timed out")
}
}
31 changes: 31 additions & 0 deletions internal/commands/config_default_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import (
"fmt"
"os"
"path/filepath"
"syscall"
"testing"
"time"

"github.com/golang/mock/gomock"
"github.com/heroku/color"
Expand Down Expand Up @@ -197,6 +199,35 @@ func testConfigDefaultBuilder(t *testing.T, when spec.G, it spec.S) {
h.AssertContains(t, outBuf.String(), "builder 'nonexisting/image' not found")
})
})

when("SIGINT is received during query", func() {
it("aborts the operation", func() {
imageName := "myremotehub.com/buildpacks/builder:latest"

mockClient.EXPECT().InspectBuilder(imageName, true, gomock.Any()).DoAndReturn(func(string, bool, ...client.BuilderInspectionModifier) (*client.BuilderInfo, error) {
time.Sleep(5 * time.Second)
return nil, nil
})

done := make(chan error, 1)
go func() {
cmd.SetArgs([]string{imageName})
done <- cmd.Execute()
}()

time.Sleep(2 * time.Second)

p, err := os.FindProcess(os.Getpid())
h.AssertNil(t, err)
err = p.Signal(syscall.SIGINT)
h.AssertNil(t, err)

err = <-done

h.AssertError(t, err, "operation aborted")
h.AssertContains(t, outBuf.String(), "operation aborted")
})
})
})
})
}
Loading