Merge pull request #10239 from cli/andyfeller/10235-non-blocking-ext-update

Make extension update check non-blocking

pkg/cmd/root/extension.go

@@ -52,20 +52,25 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex
 		},
 		// PostRun handles communicating extension release information if found
 		PostRun: func(c *cobra.Command, args []string) {
-			releaseInfo := <-updateMessageChan
-			if releaseInfo != nil {
-				stderr := io.ErrOut
-				fmt.Fprintf(stderr, "\n\n%s %s → %s\n",
-					cs.Yellowf("A new release of %s is available:", ext.Name()),
-					cs.Cyan(strings.TrimPrefix(ext.CurrentVersion(), "v")),
-					cs.Cyan(strings.TrimPrefix(releaseInfo.Version, "v")))
-				if ext.IsPinned() {
-					fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name())
-				} else {
-					fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name())
+			select {
+			case releaseInfo := <-updateMessageChan:
+				if releaseInfo != nil {
+					stderr := io.ErrOut
+					fmt.Fprintf(stderr, "\n\n%s %s → %s\n",
+						cs.Yellowf("A new release of %s is available:", ext.Name()),
+						cs.Cyan(strings.TrimPrefix(ext.CurrentVersion(), "v")),
+						cs.Cyan(strings.TrimPrefix(releaseInfo.Version, "v")))
+					if ext.IsPinned() {
+						fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s --force\n", ext.Name())
+					} else {
+						fmt.Fprintf(stderr, "To upgrade, run: gh extension upgrade %s\n", ext.Name())
+					}
+					fmt.Fprintf(stderr, "%s\n\n",
+						cs.Yellow(releaseInfo.URL))
 				}
-				fmt.Fprintf(stderr, "%s\n\n",
-					cs.Yellow(releaseInfo.URL))
+			default:
+				// Do not make the user wait for extension update check if incomplete by this time.
+				// This is being handled in non-blocking default as there is no context to cancel like in gh update checks.
 			}
 		},
 		GroupID: "extension",

pkg/cmd/root/extension_test.go

@@ -1,8 +1,10 @@
 package root_test
 
 import (
+	"fmt"
 	"io"
 	"testing"
+	"time"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/v2/internal/update"
@@ -121,6 +123,10 @@ func TestNewCmdExtension_Updates(t *testing.T) {
 		em := &extensions.ExtensionManagerMock{
 			DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) {
 				// Assume extension executed / dispatched without problems as test is focused on upgrade checking.
+				// Sleep for 100 milliseconds to allow update checking logic to complete. This would be better
+				// served by making the behaviour controllable by channels, but it's a larger change than desired
+				// just to improve the test.
+				time.Sleep(100 * time.Millisecond)
 				return true, nil
 			},
 		}
@@ -169,3 +175,62 @@ func TestNewCmdExtension_Updates(t *testing.T) {
 		}
 	}
 }
+
+func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) {
+	ios, _, _, _ := iostreams.Test()
+
+	em := &extensions.ExtensionManagerMock{
+		DispatchFunc: func(args []string, stdin io.Reader, stdout io.Writer, stderr io.Writer) (bool, error) {
+			// Assume extension executed / dispatched without problems as test is focused on upgrade checking.
+			return true, nil
+		},
+	}
+
+	ext := &extensions.ExtensionMock{
+		CurrentVersionFunc: func() string {
+			return "1.0.0"
+		},
+		IsPinnedFunc: func() bool {
+			return false
+		},
+		LatestVersionFunc: func() string {
+			return "2.0.0"
+		},
+		NameFunc: func() string {
+			return "major-update"
+		},
+		UpdateAvailableFunc: func() bool {
+			return true
+		},
+		URLFunc: func() string {
+			return "https//github.com/dne/major-update"
+		},
+	}
+
+	// When the extension command is executed, the checkFunc will run in the background longer than the extension dispatch.
+	// If the update check is non-blocking, then the extension command will complete immediately while checkFunc is still running.
+	checkFunc := func(em extensions.ExtensionManager, ext extensions.Extension) (*update.ReleaseInfo, error) {
+		time.Sleep(30 * time.Second)
+		return nil, fmt.Errorf("update check should not have completed")
+	}
+
+	cmd := root.NewCmdExtension(ios, em, ext, checkFunc)
+
+	// The test whether update check is non-blocking is based on how long it takes for the extension command execution.
+	// If there is no wait time as checkFunc is sleeping sufficiently long, we can trust update check is non-blocking.
+	// Otherwise, if any amount of wait is encountered, it is a decent indicator that update checking is blocking.
+	// This is not an ideal test and indicates the update design should be revisited to be easier to understand and manage.
+	completed := make(chan struct{})
+	go func() {
+		_, err := cmd.ExecuteC()
+		require.NoError(t, err)
+		close(completed)
+	}()
+
+	select {
+	case <-completed:
+		// Expected behavior assuming extension dispatch exits immediately while checkFunc is still running.
+	case <-time.After(1 * time.Second):
+		t.Fatal("extension update check should have exited")
+	}
+}