From df21fd28e1a239b207fb0564f28e848a1fd9bf43 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Thu, 6 Sep 2018 01:28:50 +0900 Subject: [PATCH 1/2] connhelper: fix cmd.Wait() race Fix #1336 Signed-off-by: Akihiro Suda (cherry picked from commit a22853e64db4001d9b477a380e84baaca744ff5d) Signed-off-by: Tonis Tiigi Upstream-commit: b2cf18ac2e1e2c58ce4933c196b44629d46cb64b Component: cli --- components/cli/cli/connhelper/connhelper.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/components/cli/cli/connhelper/connhelper.go b/components/cli/cli/connhelper/connhelper.go index 30c9dd8e45..3e04bc684f 100644 --- a/components/cli/cli/connhelper/connhelper.go +++ b/components/cli/cli/connhelper/connhelper.go @@ -82,6 +82,7 @@ func newCommandConn(ctx context.Context, cmd string, args ...string) (net.Conn, // commandConn implements net.Conn type commandConn struct { cmd *exec.Cmd + cmdMutex sync.Mutex stdin io.WriteCloser stdout io.ReadCloser stderrMu sync.Mutex @@ -102,6 +103,7 @@ func (c *commandConn) killIfStdioClosed() error { return nil } var err error + c.cmdMutex.Lock() // NOTE: maybe already killed here if err = c.cmd.Process.Kill(); err == nil { err = c.cmd.Wait() @@ -113,11 +115,14 @@ func (c *commandConn) killIfStdioClosed() error { err = nil } } + c.cmdMutex.Unlock() return err } func (c *commandConn) onEOF(eof error) error { + c.cmdMutex.Lock() werr := c.cmd.Wait() + c.cmdMutex.Unlock() if werr == nil { return eof } @@ -131,6 +136,7 @@ func ignorableCloseError(err error) bool { errS := err.Error() ss := []string{ os.ErrClosed.Error(), + "process already finished", } for _, s := range ss { if strings.Contains(errS, s) { @@ -148,7 +154,10 @@ func (c *commandConn) CloseRead() error { c.stdioClosedMu.Lock() c.stdoutClosed = true c.stdioClosedMu.Unlock() - return c.killIfStdioClosed() + if err := c.killIfStdioClosed(); err != nil && !ignorableCloseError(err) { + logrus.Warnf("commandConn.CloseRead: %v", err) + } + return nil } func (c *commandConn) Read(p []byte) (int, error) { @@ -167,7 +176,10 @@ func (c *commandConn) CloseWrite() error { c.stdioClosedMu.Lock() c.stdinClosed = true c.stdioClosedMu.Unlock() - return c.killIfStdioClosed() + if err := c.killIfStdioClosed(); err != nil && !ignorableCloseError(err) { + logrus.Warnf("commandConn.CloseWrite: %v", err) + } + return nil } func (c *commandConn) Write(p []byte) (int, error) { From 35e5a1ade4427116fb5202f159562d7cd50ab353 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 Sep 2018 18:35:39 +0200 Subject: [PATCH 2/2] Fix substitution with non-empty env-var Due to a typo, substitution would not work if the given environment-variable was set. Given the following docker compose file; ```yaml version: "3.7" services: app: image: nginx:${version:-latest} ``` Deploying a stack with `$version` set would ignore the `$version` environment variable, and use the default value instead; ```bash version=alpine docker stack deploy -c docker-compose.yml foobar Creating network foobar_default Creating service foobar_app docker service ls ID NAME MODE REPLICAS IMAGE PORTS rskkjxe6sm0w foobar_app replicated 1/1 nginx:latest ``` This patch also fixes "soft default" not detecting empty environment variables, only non-set environment variables. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit ec3daea0214b9600062e13413ce3062c9d3690cb) Signed-off-by: Sebastiaan van Stijn Upstream-commit: b8702b8a9a99249b060664748682f4d460de90ca Component: cli --- components/cli/cli/compose/template/template.go | 16 +++++++++++----- .../cli/cli/compose/template/template_test.go | 6 ++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/components/cli/cli/compose/template/template.go b/components/cli/cli/compose/template/template.go index 1762ab11a9..b958bde401 100644 --- a/components/cli/cli/compose/template/template.go +++ b/components/cli/cli/compose/template/template.go @@ -176,15 +176,21 @@ func extractVariable(value interface{}, pattern *regexp.Regexp) ([]extractedValu // Soft default (fall back if unset or empty) func softDefault(substitution string, mapping Mapping) (string, bool, error) { - return withDefault(substitution, mapping, "-:") + sep := ":-" + if !strings.Contains(substitution, sep) { + return "", false, nil + } + name, defaultValue := partition(substitution, sep) + value, ok := mapping(name) + if !ok || value == "" { + return defaultValue, true, nil + } + return value, true, nil } // Hard default (fall back if-and-only-if empty) func hardDefault(substitution string, mapping Mapping) (string, bool, error) { - return withDefault(substitution, mapping, "-") -} - -func withDefault(substitution string, mapping Mapping, sep string) (string, bool, error) { + sep := "-" if !strings.Contains(substitution, sep) { return "", false, nil } diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index 672a7e75d9..ce3690410f 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -78,6 +78,12 @@ func TestEmptyValueWithSoftDefault(t *testing.T) { assert.Check(t, is.Equal("ok def", result)) } +func TestValueWithSoftDefault(t *testing.T) { + result, err := Substitute("ok ${FOO:-def}", defaultMapping) + assert.NilError(t, err) + assert.Check(t, is.Equal("ok first", result)) +} + func TestEmptyValueWithHardDefault(t *testing.T) { result, err := Substitute("ok ${BAR-def}", defaultMapping) assert.NilError(t, err)