Skip to content

cli: set timeout connection ping on sockets as well#3722

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

Merged
thaJeztah merged 1 commit into from
Aug 3, 2022
Merged

cli: set timeout connection ping on sockets as well #3722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 12 additions & 2 deletions cli/command/cli.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -35,6 +35,8 @@ import (
notaryclient "github.com/theupdateframework/notary/client"
)

const defaultInitTimeout = 2 * time.Second

// Streams is an interface which exposes the standard input and output streams
type Streams interface {
In() *streams.In
Expand DownExpand Up@@ -77,6 +79,7 @@ type DockerCli struct {
currentContext string
dockerEndpoint docker.Endpoint
contextStoreConfig store.Config
initTimeout time.Duration
}

// DefaultVersion returns api.defaultVersion.
Expand DownExpand Up@@ -313,13 +316,20 @@ func resolveDefaultDockerEndpoint(opts *cliflags.CommonOptions) (docker.Endpoint
}, nil
}

func (cli *DockerCli) getInitTimeout() time.Duration {
if cli.initTimeout != 0 {
return cli.initTimeout
}
return defaultInitTimeout
}

func (cli *DockerCli) initializeFromClient() {
ctx := context.Background()
if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
if !strings.HasPrefix(cli.DockerEndpoint().Host, "ssh://") {
// @FIXME context.WithTimeout doesn't work with connhelper / ssh connections
// time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed"

@vvolandvvolandJul 29, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know it isn't a part of this PR, but maybe you remember how does context.WithTimeout has anything to do with ssh being used? The PR that introduced this doesn't mention any details here.

@vvolandvvolandJul 29, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just curious - so if you remember then just some quick tldr will be enough 😄

@thaJeztahthaJeztahJul 29, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm not sure how much time I spent at the time digging into the exact cause; the timeout was helping with an issue (see #2424), and I'm not sure I was able to consistently reproduce the ssh problem locally (or only in CI?). Ultimately decided that the SSH connections were less common, so to keep diving into the issue for later (there's some other issues remaining with the ssh connection helper as well).

In case useful;

@thaJeztahthaJeztahJul 29, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It may be just missing functionality in the ssh connecting helper (it not handling context cancelation), but yeah, never found the time really to get myself familiar with that code or to dig into the issue.

@vvolandvvolandAug 1, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

var cancel func()
ctx, cancel = context.WithTimeout(ctx, 2*time.Second)
ctx, cancel = context.WithTimeout(ctx, cli.getInitTimeout())
defer cancel()
}

Expand Down
53 changes: 53 additions & 0 deletions cli/command/cli_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -5,12 +5,15 @@ import (
"context"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"runtime"
"strings"
"testing"
"time"

"github.com/docker/cli/cli/config"
"github.com/docker/cli/cli/config/configfile"
Expand DownExpand Up@@ -165,6 +168,56 @@ func TestInitializeFromClient(t *testing.T) {
}
}

// Makes sure we don't hang forever on the initial connection.
// https://github.com/docker/cli/issues/3652
func TestInitializeFromClientHangs(t *testing.T) {
dir := t.TempDir()
socket := filepath.Join(dir, "my.sock")
l, err := net.Listen("unix", socket)
assert.NilError(t, err)

receiveReqCh := make(chan bool)
timeoutCtx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

// Simulate a server that hangs on connections.
ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
select {
case <-timeoutCtx.Done():
case receiveReqCh <- true: // Blocks until someone receives on the channel.
}
_, _ = w.Write([]byte("OK"))
}))
ts.Listener = l
ts.Start()
defer ts.Close()

opts := &flags.CommonOptions{Hosts: []string{fmt.Sprintf("unix://%s", socket)}}
configFile := &configfile.ConfigFile{}
apiClient, err := NewAPIClientFromFlags(opts, configFile)
assert.NilError(t, err)

initializedCh := make(chan bool)

go func() {
cli := &DockerCli{client: apiClient, initTimeout: time.Millisecond}
cli.Initialize(flags.NewClientOptions())
close(initializedCh)
}()

select {
case <-timeoutCtx.Done():
t.Fatal("timeout waiting for initialization to complete")
case <-initializedCh:
}

select {
case <-timeoutCtx.Done():
t.Fatal("server never received an init request")
case <-receiveReqCh:
}
}

// The CLI no longer disables/hides experimental CLI features, however, we need
// to verify that existing configuration files do not break
func TestExperimentalCLI(t *testing.T) {
Expand Down