From 7aaea283caf61dc89eadd417c3535784705c7918 Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Wed, 22 Mar 2023 18:05:56 -0400 Subject: [PATCH 1/2] watch: data race / segfault fixes Was getting segfaults with multiple services using `x-develop` and `watch` at the same time. Turns out the Moby path matcher lazily initializes the regex pattern internally the first time it's used, so it's not goroutine-safe. Change here is to not use a global instance for the ephemeral path matcher, but a per-watcher instance. Additionally, the data race detector caught a couple other issues that were easy enough to fix: * Use the lock that's used elsewhere for convergence before manipulating * Eliminate concurrent map access when triggering rebuilds Signed-off-by: Milas Bowman --- pkg/compose/convergence.go | 3 +++ pkg/compose/watch.go | 16 ++++++++-------- pkg/watch/ephemeral.go | 4 +--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/compose/convergence.go b/pkg/compose/convergence.go index 5fbebd4c70..eda1b45d24 100644 --- a/pkg/compose/convergence.go +++ b/pkg/compose/convergence.go @@ -455,6 +455,9 @@ func (s *composeService) recreateContainer(ctx context.Context, project *types.P // setDependentLifecycle define the Lifecycle strategy for all services to depend on specified service func setDependentLifecycle(project *types.Project, service string, strategy string) { + mu.Lock() + defer mu.Unlock() + for i, s := range project.Services { if utils.StringContains(s.GetDependencies(), service) { if s.Extensions == nil { diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index 00d60bd1be..79b7e2d5b2 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -133,7 +133,7 @@ func (s *composeService) Watch(ctx context.Context, project *types.Project, serv } ignore := watch.NewCompositeMatcher( dockerIgnores, - watch.EphemeralPathMatcher, + watch.EphemeralPathMatcher(), dotGitIgnore, ) @@ -336,17 +336,17 @@ type rebuildServices map[string]utils.Set[string] func debounce(ctx context.Context, clock clockwork.Clock, delay time.Duration, input <-chan fileMapping, fn func(services rebuildServices)) { services := make(rebuildServices) - t := clock.AfterFunc(delay, func() { - if len(services) > 0 { - fn(services) - // TODO(milas): this is a data race! - services = make(rebuildServices) - } - }) + t := clock.NewTimer(delay) + defer t.Stop() for { select { case <-ctx.Done(): return + case <-t.Chan(): + if len(services) > 0 { + go fn(services) + services = make(rebuildServices) + } case e := <-input: t.Reset(delay) svc, ok := services[e.Service] diff --git a/pkg/watch/ephemeral.go b/pkg/watch/ephemeral.go index 2aa271503c..98538abe56 100644 --- a/pkg/watch/ephemeral.go +++ b/pkg/watch/ephemeral.go @@ -25,9 +25,7 @@ package watch // there or aren't in the right places. // // https://app.clubhouse.io/windmill/story/691/filter-out-ephemeral-file-changes -var EphemeralPathMatcher = initEphemeralPathMatcher() - -func initEphemeralPathMatcher() PathMatcher { +func EphemeralPathMatcher() PathMatcher { golandPatterns := []string{"**/*___jb_old___", "**/*___jb_tmp___", "**/.idea/**"} emacsPatterns := []string{"**/.#*", "**/#*#"} // if .swp is taken (presumably because multiple vims are running in that dir), From 16d5354d709a9681c9d07e2d2129863fcd5f09bc Mon Sep 17 00:00:00 2001 From: Milas Bowman Date: Thu, 23 Mar 2023 09:49:55 -0400 Subject: [PATCH 2/2] watch: add note about goroutine-safety & test Signed-off-by: Milas Bowman --- pkg/watch/ephemeral.go | 3 ++- pkg/watch/ephemeral_test.go | 49 +++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 pkg/watch/ephemeral_test.go diff --git a/pkg/watch/ephemeral.go b/pkg/watch/ephemeral.go index 98538abe56..c7a82f5eda 100644 --- a/pkg/watch/ephemeral.go +++ b/pkg/watch/ephemeral.go @@ -24,7 +24,8 @@ package watch // stop-gap so they don't have a terrible experience if those files aren't // there or aren't in the right places. // -// https://app.clubhouse.io/windmill/story/691/filter-out-ephemeral-file-changes +// NOTE: The underlying `patternmatcher` is NOT always Goroutine-safe, so +// this is not a singleton; we create an instance for each watcher currently. func EphemeralPathMatcher() PathMatcher { golandPatterns := []string{"**/*___jb_old___", "**/*___jb_tmp___", "**/.idea/**"} emacsPatterns := []string{"**/.#*", "**/#*#"} diff --git a/pkg/watch/ephemeral_test.go b/pkg/watch/ephemeral_test.go new file mode 100644 index 0000000000..1d9525723d --- /dev/null +++ b/pkg/watch/ephemeral_test.go @@ -0,0 +1,49 @@ +/* +Copyright 2023 Docker Compose CLI authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package watch_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/docker/compose/v2/pkg/watch" +) + +func TestEphemeralPathMatcher(t *testing.T) { + ignored := []string{ + ".file.txt.swp", + "/path/file.txt~", + "/home/moby/proj/.idea/modules.xml", + ".#file.txt", + "#file.txt#", + "/dir/.file.txt.kate-swp", + "/go/app/1234-go-tmp-umask", + } + matcher := watch.EphemeralPathMatcher() + for _, p := range ignored { + ok, err := matcher.Matches(p) + if assert.NoErrorf(t, err, "Matching %s", p) { + assert.Truef(t, ok, "Path %s should have matched", p) + } + } + + const includedPath = "normal.txt" + ok, err := matcher.Matches(includedPath) + if assert.NoErrorf(t, err, "Matching %s", includedPath) { + assert.Falsef(t, ok, "Path %s should NOT have matched", includedPath) + } +}