From 7f682f9c398099f30ab0824db56afc95a9edce9e Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Mon, 20 Sep 2021 16:56:57 -0400 Subject: [PATCH 1/3] Close Live Share sessions - New helper method codespaces.CloseSession to be used using defer - Upgrade to go-liveshare v0.17.0 --- cmd/ghcs/logs.go | 3 ++- cmd/ghcs/ports.go | 9 ++++++--- cmd/ghcs/ssh.go | 3 ++- internal/codespaces/codespaces.go | 8 ++++++++ internal/codespaces/states.go | 3 ++- 5 files changed, 20 insertions(+), 6 deletions(-) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 19528061a..4a38319e6 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -40,7 +40,7 @@ func init() { rootCmd.AddCommand(newLogsCmd()) } -func logs(ctx context.Context, log *output.Logger, codespaceName string, follow bool) error { +func logs(ctx context.Context, log *output.Logger, codespaceName string, follow bool) (err error) { // Ensure all child tasks (port forwarding, remote exec) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -61,6 +61,7 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow if err != nil { return fmt.Errorf("connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 7bc53c441..45f92da7d 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -51,7 +51,7 @@ func init() { rootCmd.AddCommand(newPortsCmd()) } -func ports(codespaceName string, asJSON bool) error { +func ports(codespaceName string, asJSON bool) (err error) { apiClient := api.New(os.Getenv("GITHUB_TOKEN")) ctx := context.Background() log := output.NewLogger(os.Stdout, os.Stderr, asJSON) @@ -76,6 +76,7 @@ func ports(codespaceName string, asJSON bool) error { if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) log.Println("Loading ports...") ports, err := session.GetSharedServers(ctx) @@ -198,7 +199,7 @@ func newPortsPrivateCmd() *cobra.Command { } } -func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) error { +func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, public bool) (err error) { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -219,6 +220,7 @@ func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) port, err := strconv.Atoi(sourcePort) if err != nil { @@ -260,7 +262,7 @@ func newPortsForwardCmd() *cobra.Command { } } -func forwardPorts(log *output.Logger, codespaceName string, ports []string) error { +func forwardPorts(log *output.Logger, codespaceName string, ports []string) (err error) { ctx := context.Background() apiClient := api.New(os.Getenv("GITHUB_TOKEN")) @@ -286,6 +288,7 @@ func forwardPorts(log *output.Logger, codespaceName string, ports []string) erro if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) // Run forwarding of all ports concurrently, aborting all of // them at the first failure, including cancellation of the context. diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index 4ece84d91..a92c99bb3 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -36,7 +36,7 @@ func init() { rootCmd.AddCommand(newSSHCmd()) } -func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string, localSSHServerPort int) error { +func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string, localSSHServerPort int) (err error) { // Ensure all child tasks (e.g. port forwarding) terminate before return. ctx, cancel := context.WithCancel(ctx) defer cancel() @@ -58,6 +58,7 @@ func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } + defer codespaces.CloseSession(session, &err) log.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index 6235ca3a0..fe62a3d79 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -73,3 +73,11 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } + +// CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. +func CloseSession(session *liveshare.Session, err *error) { + closeErr := session.Close() + if *err == nil { + *err = closeErr + } +} diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 408f11941..7e464d919 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -36,7 +36,7 @@ type PostCreateState struct { // PollPostCreateStates watches for state changes in a codespace, // and calls the supplied poller for each batch of state changes. // It runs until it encounters an error, including cancellation of the context. -func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, user *api.User, codespace *api.Codespace, poller func([]PostCreateState)) error { +func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, user *api.User, codespace *api.Codespace, poller func([]PostCreateState)) (err error) { token, err := apiClient.GetCodespaceToken(ctx, user.Login, codespace.Name) if err != nil { return fmt.Errorf("getting codespace token: %w", err) @@ -46,6 +46,7 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u if err != nil { return fmt.Errorf("connect to Live Share: %w", err) } + defer CloseSession(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port From a83b3c08167cddb3f8edb07c25ad1de428194c79 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 08:46:32 -0400 Subject: [PATCH 2/3] Update to go-livesare v0.18.0 - Only set err if closeErr is non-nil --- internal/codespaces/codespaces.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index fe62a3d79..ae1115905 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -23,6 +23,8 @@ func connectionReady(codespace *api.Codespace) bool { codespace.Environment.State == api.CodespaceEnvironmentStateAvailable } +// ConnectToLiveshare creates a Live Share client and joins the Live Share session. +// It will start the Codespace if it is not already running, it will time out after 60 seconds if fails to start. func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, userLogin, token string, codespace *api.Codespace) (*liveshare.Session, error) { var startedCodespace bool if codespace.Environment.State != api.CodespaceEnvironmentStateAvailable { @@ -75,9 +77,10 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use } // CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. +// It is meant to be called using defer with a named return argument for the error. func CloseSession(session *liveshare.Session, err *error) { closeErr := session.Close() - if *err == nil { - *err = closeErr + if *err == nil && closeErr != nil { + *err = fmt.Errorf("failed to close Live Share session: %w", closeErr) } } From e8e914c220b9ec828b4965a07a843d67bb4c3c18 Mon Sep 17 00:00:00 2001 From: Jose Garcia Date: Tue, 21 Sep 2021 10:05:48 -0400 Subject: [PATCH 3/3] PR Feedback - Upgrade to go-liveshare v0.19.0 - Remove export helper method - Use local implementation --- cmd/ghcs/common.go | 7 +++++++ cmd/ghcs/logs.go | 2 +- cmd/ghcs/ports.go | 6 +++--- cmd/ghcs/ssh.go | 2 +- internal/codespaces/codespaces.go | 9 --------- internal/codespaces/states.go | 6 +++++- 6 files changed, 17 insertions(+), 15 deletions(-) diff --git a/cmd/ghcs/common.go b/cmd/ghcs/common.go index e71e3dfe4..79fda32ef 100644 --- a/cmd/ghcs/common.go +++ b/cmd/ghcs/common.go @@ -6,6 +6,7 @@ import ( "context" "errors" "fmt" + "io" "os" "sort" @@ -93,6 +94,12 @@ func getOrChooseCodespace(ctx context.Context, apiClient *api.API, user *api.Use return codespace, token, nil } +func safeClose(closer io.Closer, err *error) { + if closeErr := closer.Close(); *err == nil { + *err = closeErr + } +} + // hasTTY indicates whether the process connected to a terminal. // It is not portable to assume stdin/stdout are fds 0 and 1. var hasTTY = term.IsTerminal(int(os.Stdin.Fd())) && term.IsTerminal(int(os.Stdout.Fd())) diff --git a/cmd/ghcs/logs.go b/cmd/ghcs/logs.go index 4a38319e6..db83250c5 100644 --- a/cmd/ghcs/logs.go +++ b/cmd/ghcs/logs.go @@ -61,7 +61,7 @@ func logs(ctx context.Context, log *output.Logger, codespaceName string, follow if err != nil { return fmt.Errorf("connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port diff --git a/cmd/ghcs/ports.go b/cmd/ghcs/ports.go index 45f92da7d..f48dd1e6d 100644 --- a/cmd/ghcs/ports.go +++ b/cmd/ghcs/ports.go @@ -76,7 +76,7 @@ func ports(codespaceName string, asJSON bool) (err error) { if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) log.Println("Loading ports...") ports, err := session.GetSharedServers(ctx) @@ -220,7 +220,7 @@ func updatePortVisibility(log *output.Logger, codespaceName, sourcePort string, if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) port, err := strconv.Atoi(sourcePort) if err != nil { @@ -288,7 +288,7 @@ func forwardPorts(log *output.Logger, codespaceName string, ports []string) (err if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) // Run forwarding of all ports concurrently, aborting all of // them at the first failure, including cancellation of the context. diff --git a/cmd/ghcs/ssh.go b/cmd/ghcs/ssh.go index a92c99bb3..88117c480 100644 --- a/cmd/ghcs/ssh.go +++ b/cmd/ghcs/ssh.go @@ -58,7 +58,7 @@ func ssh(ctx context.Context, sshArgs []string, sshProfile, codespaceName string if err != nil { return fmt.Errorf("error connecting to Live Share: %w", err) } - defer codespaces.CloseSession(session, &err) + defer safeClose(session, &err) log.Println("Fetching SSH Details...") remoteSSHServerPort, sshUser, err := session.StartSSHServer(ctx) diff --git a/internal/codespaces/codespaces.go b/internal/codespaces/codespaces.go index ae1115905..2933c9d8d 100644 --- a/internal/codespaces/codespaces.go +++ b/internal/codespaces/codespaces.go @@ -75,12 +75,3 @@ func ConnectToLiveshare(ctx context.Context, log logger, apiClient *api.API, use return lsclient.JoinWorkspace(ctx) } - -// CloseSession closes the Live Share session and assigns the error to the pointer if it is nil. -// It is meant to be called using defer with a named return argument for the error. -func CloseSession(session *liveshare.Session, err *error) { - closeErr := session.Close() - if *err == nil && closeErr != nil { - *err = fmt.Errorf("failed to close Live Share session: %w", closeErr) - } -} diff --git a/internal/codespaces/states.go b/internal/codespaces/states.go index 7e464d919..31105d576 100644 --- a/internal/codespaces/states.go +++ b/internal/codespaces/states.go @@ -46,7 +46,11 @@ func PollPostCreateStates(ctx context.Context, log logger, apiClient *api.API, u if err != nil { return fmt.Errorf("connect to Live Share: %w", err) } - defer CloseSession(session, &err) + defer func() { + if closeErr := session.Close(); err == nil { + err = closeErr + } + }() // Ensure local port is listening before client (getPostCreateOutput) connects. listen, err := net.Listen("tcp", ":0") // arbitrary port