Skip to content

Commit d0968a8

Browse files
committed
cmd/create: Don't block user interaction while fetching the image size
It takes 'skopeo inspect' a few seconds to fetch the image size from the remote registry, and while that happens the user can't interact with the image download prompt: $ toolbox create Image required to create toolbox container. <wait for a few seconds> Download registry.fedoraproject.org/fedora-toolbox:39 (359.8MB)? [y/N]: This feels awkward because it's not clear to the user what's going on during those few seconds. Moreover, while knowing the image size can be convenient at times, for example when disk space and network bandwidth are limited, it's not always important. It will be better if 'skopeo inspect' ran in the background, while waiting for the user to respond to the image download prompt, and once the image size has been fetched, the image download prompt can be updated to include it. So, initially: $ toolbox create Image required to create toolbox container. Download registry.fedoraproject.org/fedora-toolbox:39 (...)? [y/N]: ... and then once the size is available: $ toolbox create Image required to create toolbox container. Download registry.fedoraproject.org/fedora-toolbox:39 (359.8MB)? [y/N]: If skopeo(1) is missing or too old, then the prompt can continue without the size, as it did before: $ toolbox create Image required to create toolbox container. Download registry.fedoraproject.org/fedora-toolbox:39 [y/N]: Updating the image download prompt with the results of 'skopeo inspect' is vulnerable to races. At the same time as the terminal's cursor is moved to the beginning of the current line to overwrite the earlier prompt with the new one, the user can keep typing, which will keep moving the cursor forward. This competition over the cursor can lead to awkward outcomes. For example, the prompt can overwrite the characters typed in by the user, leaving characters in the terminal's input buffer waiting for the user to hit ENTER, even though they are not visible on the screen. Another example is that hitting BACKSPACE can end up deleting parts of the prompt, instead of stopping at the edge. This is solved by putting the terminal device into non-canonical mode input and disabling the echoing of input characters, while the prompt is being updated. This prevents input from moving the terminal's cursor forward, and from accumulating in the terminal's input buffer even if it might not be visible. Any input during this interim period is discarded and replaced by '...', and a fresh new prompt is shown in the following line. In practice, this race shouldn't be too common. It can only happen if the user is typing right when the prompt is being updated, which is unlikely because it's only supposed to be a short 'yes' or 'no' input. The use of the context.Cause and context.WithCancelCause functions [1] requires Go >= 1.20. Bumping the Go version in src/go.mod then requires a 'go mod tidy'. Otherwise, it leads to: $ meson compile -C builddir --verbose ... /home/rishi/devel/containers/git/toolbox/src/go-build-wrapper /home/rishi/devel/containers/git/toolbox/src /home/rishi/devel/containers/git/toolbox/builddir src/toolbox 0.0.99.4 cc /lib64/ld-linux-x86-64.so.2 false go: updates to go.mod needed; to update it: go mod tidy ninja: build stopped: subcommand failed. [1] https://pkg.go.dev/context #752 #1263
1 parent 878fdf4 commit d0968a8

File tree

4 files changed

+313
-21
lines changed

4 files changed

+313
-21
lines changed

.github/workflows/ubuntu-tests.yaml

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ jobs:
4444
fish \
4545
gcc \
4646
go-md2man \
47-
golang \
47+
golang-1.20 \
4848
meson \
4949
ninja-build \
5050
openssl \
@@ -54,6 +54,10 @@ jobs:
5454
systemd \
5555
udisks2
5656
57+
- name: Set up PATH for Go 1.20
58+
run: |
59+
echo "PATH=/usr/lib/go-1.20/bin:$PATH" >> "$GITHUB_ENV"
60+
5761
- name: Checkout Bats
5862
uses: actions/checkout@v3
5963
with:

src/cmd/create.go

Lines changed: 285 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,16 @@ import (
3737
"github.com/spf13/cobra"
3838
)
3939

40+
type promptForDownloadError struct {
41+
ImageSize string
42+
}
43+
44+
type promptOptions struct {
45+
UsePlaceholder bool
46+
}
47+
48+
type promptOption func(*promptOptions)
49+
4050
const (
4151
alpha = `abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ`
4252
num = `0123456789`
@@ -573,8 +583,7 @@ func getFullyQualifiedImageFromRepoTags(image string) (string, error) {
573583
return imageFull, nil
574584
}
575585

576-
func getImageSizeFromRegistry(imageFull string) (string, error) {
577-
ctx := context.Background()
586+
func getImageSizeFromRegistry(ctx context.Context, imageFull string) (string, error) {
578587
image, err := skopeo.Inspect(ctx, imageFull)
579588
if err != nil {
580589
return "", err
@@ -598,6 +607,23 @@ func getImageSizeFromRegistry(imageFull string) (string, error) {
598607
return imageSizeHuman, nil
599608
}
600609

610+
func getImageSizeFromRegistryAsync(ctx context.Context, imageFull string) (<-chan string, <-chan error) {
611+
retValCh := make(chan string)
612+
errCh := make(chan error)
613+
614+
go func() {
615+
imageSize, err := getImageSizeFromRegistry(ctx, imageFull)
616+
if err != nil {
617+
errCh <- err
618+
return
619+
}
620+
621+
retValCh <- imageSize
622+
}()
623+
624+
return retValCh, errCh
625+
}
626+
601627
func getServiceSocket(serviceName string, unitName string) (string, error) {
602628
logrus.Debugf("Resolving path to the %s socket", serviceName)
603629

@@ -710,18 +736,7 @@ func pullImage(image, release, authFile string) (bool, error) {
710736
}
711737

712738
if promptForDownload {
713-
fmt.Println("Image required to create toolbox container.")
714-
715-
var prompt string
716-
717-
if imageSize, err := getImageSizeFromRegistry(imageFull); err != nil {
718-
logrus.Debugf("Getting image size failed: %s", err)
719-
prompt = fmt.Sprintf("Download %s? [y/N]:", imageFull)
720-
} else {
721-
prompt = fmt.Sprintf("Download %s (%s)? [y/N]:", imageFull, imageSize)
722-
}
723-
724-
shouldPullImage = askForConfirmation(prompt)
739+
shouldPullImage = showPromptForDownload(imageFull)
725740
}
726741

727742
if !shouldPullImage {
@@ -751,6 +766,248 @@ func pullImage(image, release, authFile string) (bool, error) {
751766
return true, nil
752767
}
753768

769+
func createPromptForDownload(imageFull, imageSize string, options ...promptOption) string {
770+
o := &promptOptions{}
771+
for _, option := range options {
772+
option(o)
773+
}
774+
775+
if imageSize == "" {
776+
if o.UsePlaceholder {
777+
imageSize = " ... MB"
778+
} else {
779+
prompt := fmt.Sprintf("Download %s? [y/N]:", imageFull)
780+
return prompt
781+
}
782+
}
783+
784+
imageSizeLen := len(imageSize)
785+
var padding1 int
786+
var padding2 int
787+
var padding3 int
788+
var padding4 int
789+
790+
if imageSizeLen < 7 {
791+
padding4 = 7 - imageSizeLen
792+
}
793+
794+
if padding4 > 1 {
795+
padding3 = padding4 - 1
796+
padding4 = 1
797+
}
798+
799+
if padding3 > 1 {
800+
padding2 = padding3 - 1
801+
padding3 = 1
802+
}
803+
804+
if padding2 > 1 {
805+
padding1 = padding2 - 1
806+
padding2 = 1
807+
}
808+
809+
prompt := fmt.Sprintf("Download %s (%*s%s%*s)? %*s[y/N]:%*s",
810+
imageFull,
811+
padding1, "",
812+
imageSize,
813+
padding2, "",
814+
padding3, "",
815+
padding4, "")
816+
817+
return prompt
818+
}
819+
820+
func showPromptForDownloadFirst(imageFull string) (bool, error) {
821+
withPlaceholder := func(o *promptOptions) {
822+
o.UsePlaceholder = true
823+
}
824+
825+
prompt := createPromptForDownload(imageFull, "", withPlaceholder)
826+
827+
parentCtx := context.Background()
828+
askCtx, askCancel := context.WithCancelCause(parentCtx)
829+
defer askCancel(errors.New("clean-up"))
830+
831+
askCh, askErrCh := askForConfirmationAsync(askCtx, prompt, nil)
832+
833+
imageSizeCtx, imageSizeCancel := context.WithCancelCause(parentCtx)
834+
defer imageSizeCancel(errors.New("clean-up"))
835+
836+
imageSizeCh, imageSizeErrCh := getImageSizeFromRegistryAsync(imageSizeCtx, imageFull)
837+
838+
var imageSize string
839+
var shouldPullImage bool
840+
841+
select {
842+
case val := <-askCh:
843+
shouldPullImage = val
844+
cause := fmt.Errorf("%w: received confirmation without image size", context.Canceled)
845+
imageSizeCancel(cause)
846+
case err := <-askErrCh:
847+
shouldPullImage = false
848+
cause := fmt.Errorf("failed to ask for confirmation without image size: %w", err)
849+
imageSizeCancel(cause)
850+
case val := <-imageSizeCh:
851+
imageSize = val
852+
cause := fmt.Errorf("%w: received image size", context.Canceled)
853+
askCancel(cause)
854+
case err := <-imageSizeErrCh:
855+
cause := fmt.Errorf("failed to get image size: %w", err)
856+
askCancel(cause)
857+
}
858+
859+
if imageSizeCtx.Err() != nil && askCtx.Err() == nil {
860+
cause := context.Cause(imageSizeCtx)
861+
logrus.Debugf("Show prompt for download: image size canceled: %s", cause)
862+
return shouldPullImage, nil
863+
}
864+
865+
var done bool
866+
867+
if imageSizeCtx.Err() == nil && askCtx.Err() != nil {
868+
select {
869+
case val := <-askCh:
870+
logrus.Debugf("Show prompt for download: received pending confirmation without image size")
871+
shouldPullImage = val
872+
done = true
873+
case err := <-askErrCh:
874+
logrus.Debugf("Show prompt for download: failed to ask for confirmation without image size: %s",
875+
err)
876+
}
877+
} else {
878+
panic("code should not be reached")
879+
}
880+
881+
cause := context.Cause(askCtx)
882+
logrus.Debugf("Show prompt for download: ask canceled: %s", cause)
883+
884+
if done {
885+
return shouldPullImage, nil
886+
}
887+
888+
return false, &promptForDownloadError{imageSize}
889+
}
890+
891+
func showPromptForDownloadSecond(imageFull string, errFirst *promptForDownloadError) bool {
892+
oldState, err := term.GetState(os.Stdin)
893+
if err != nil {
894+
logrus.Debugf("Show prompt for download: failed to get terminal state: %s", err)
895+
return false
896+
}
897+
898+
defer term.SetState(os.Stdin, oldState)
899+
900+
lockedState := term.NewStateFrom(oldState,
901+
term.WithVMIN(1),
902+
term.WithVTIME(0),
903+
term.WithoutECHO(),
904+
term.WithoutICANON())
905+
906+
if err := term.SetState(os.Stdin, lockedState); err != nil {
907+
logrus.Debugf("Show prompt for download: failed to set terminal state: %s", err)
908+
return false
909+
}
910+
911+
parentCtx := context.Background()
912+
discardCtx, discardCancel := context.WithCancelCause(parentCtx)
913+
defer discardCancel(errors.New("clean-up"))
914+
915+
discardCh, discardErrCh := discardInputAsync(discardCtx)
916+
917+
var prompt string
918+
if errors.Is(errFirst, context.Canceled) {
919+
prompt = createPromptForDownload(imageFull, errFirst.ImageSize)
920+
} else {
921+
prompt = createPromptForDownload(imageFull, "")
922+
}
923+
924+
fmt.Printf("\r")
925+
926+
askCtx, askCancel := context.WithCancelCause(parentCtx)
927+
defer askCancel(errors.New("clean-up"))
928+
929+
var askForConfirmationPreFnDone bool
930+
askForConfirmationPreFn := func() error {
931+
defer discardCancel(errors.New("clean-up"))
932+
if askForConfirmationPreFnDone {
933+
return nil
934+
}
935+
936+
// Save the cursor position.
937+
fmt.Printf("\033[s")
938+
939+
if err := term.SetState(os.Stdin, oldState); err != nil {
940+
return fmt.Errorf("failed to restore terminal state: %w", err)
941+
}
942+
943+
cause := errors.New("terminal restored")
944+
discardCancel(cause)
945+
946+
err := <-discardErrCh
947+
if !errors.Is(err, context.Canceled) {
948+
return fmt.Errorf("failed to discard input: %w", err)
949+
}
950+
951+
logrus.Debugf("Show prompt for download: stopped discarding input: %s", err)
952+
953+
discardTotal := <-discardCh
954+
logrus.Debugf("Show prompt for download: discarded input: %d bytes", discardTotal)
955+
956+
if discardTotal == 0 {
957+
askForConfirmationPreFnDone = true
958+
return nil
959+
}
960+
961+
if err := term.SetState(os.Stdin, lockedState); err != nil {
962+
return fmt.Errorf("failed to set terminal state: %w", err)
963+
}
964+
965+
discardCtx, discardCancel = context.WithCancelCause(parentCtx)
966+
discardCh, discardErrCh = discardInputAsync(discardCtx)
967+
968+
// Restore the cursor position
969+
fmt.Printf("\033[u")
970+
971+
// Erase to end of line
972+
fmt.Printf("\033[K")
973+
974+
fmt.Printf("...\n")
975+
return errContinue
976+
}
977+
978+
askCh, askErrCh := askForConfirmationAsync(askCtx, prompt, askForConfirmationPreFn)
979+
var shouldPullImage bool
980+
981+
select {
982+
case val := <-askCh:
983+
logrus.Debug("Show prompt for download: received confirmation with image size")
984+
shouldPullImage = val
985+
case err := <-askErrCh:
986+
logrus.Debugf("Show prompt for download: failed to ask for confirmation with image size: %s", err)
987+
shouldPullImage = false
988+
}
989+
990+
return shouldPullImage
991+
}
992+
993+
func showPromptForDownload(imageFull string) bool {
994+
fmt.Println("Image required to create toolbox container.")
995+
996+
shouldPullImage, err := showPromptForDownloadFirst(imageFull)
997+
if err == nil {
998+
return shouldPullImage
999+
}
1000+
1001+
var errPromptForDownload *promptForDownloadError
1002+
if !errors.As(err, &errPromptForDownload) {
1003+
panicMsg := fmt.Sprintf("unexpected %T: %s", err, err)
1004+
panic(panicMsg)
1005+
}
1006+
1007+
shouldPullImage = showPromptForDownloadSecond(imageFull, errPromptForDownload)
1008+
return shouldPullImage
1009+
}
1010+
7541011
// systemdNeedsEscape checks whether a byte in a potential dbus ObjectPath needs to be escaped
7551012
func systemdNeedsEscape(i int, b byte) bool {
7561013
// Escape everything that is not a-z-A-Z-0-9
@@ -778,3 +1035,17 @@ func systemdPathBusEscape(path string) string {
7781035
}
7791036
return string(n)
7801037
}
1038+
1039+
func (err *promptForDownloadError) Error() string {
1040+
innerErr := err.Unwrap()
1041+
errMsg := innerErr.Error()
1042+
return errMsg
1043+
}
1044+
1045+
func (err *promptForDownloadError) Unwrap() error {
1046+
if err.ImageSize == "" {
1047+
return errors.New("failed to get image size")
1048+
}
1049+
1050+
return context.Canceled
1051+
}

src/go.mod

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
module github.com/containers/toolbox
22

3-
go 1.14
3+
go 1.20
44

55
require (
66
github.com/HarryMichal/go-version v1.0.1
@@ -15,3 +15,25 @@ require (
1515
github.com/stretchr/testify v1.7.0
1616
golang.org/x/sys v0.1.0
1717
)
18+
19+
require (
20+
github.com/davecgh/go-spew v1.1.1 // indirect
21+
github.com/fatih/color v1.13.0 // indirect
22+
github.com/hashicorp/hcl v1.0.0 // indirect
23+
github.com/inconshreveable/mousetrap v1.0.0 // indirect
24+
github.com/magiconair/properties v1.8.5 // indirect
25+
github.com/mattn/go-colorable v0.1.12 // indirect
26+
github.com/mattn/go-isatty v0.0.14 // indirect
27+
github.com/mitchellh/mapstructure v1.4.3 // indirect
28+
github.com/pelletier/go-toml v1.9.4 // indirect
29+
github.com/pmezard/go-difflib v1.0.0 // indirect
30+
github.com/spf13/afero v1.6.0 // indirect
31+
github.com/spf13/cast v1.4.1 // indirect
32+
github.com/spf13/jwalterweatherman v1.1.0 // indirect
33+
github.com/spf13/pflag v1.0.5 // indirect
34+
github.com/subosito/gotenv v1.2.0 // indirect
35+
golang.org/x/text v0.3.7 // indirect
36+
gopkg.in/ini.v1 v1.66.2 // indirect
37+
gopkg.in/yaml.v2 v2.4.0 // indirect
38+
gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b // indirect
39+
)

0 commit comments

Comments
 (0)