[ndeloof]

What I did

Related issue
fixes #13561

(not mandatory) A picture of a cute animal, if possible in relation to what you did

[thaJeztah]

I see there's also a Start() - should the closeOnce be reset in Start() (otherwise it won't be run again after restarting?)

(not sure if that scenario is possible)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[thaJeztah]

LGTM

but left two comments / suggestions (perhaps adding the test would make sense for this PR).

[thaJeztah]

LOL, not a new problem, but now starting to wonder if either Start() should be protected too only run once 🙈 - calling start multiple times wouldn't work well

That's not a new problem though, but perhaps something to look into.

That made me wonder if there's cases where "create watcher" and "start watcher" should be separate; looks like it's used here, and calls Start immediately after NewWatcher (wondering if Start should be done as part of the constructor);

compose/pkg/compose/watch.go

Lines 271 to 279 in bf86ba0

	watcher, err := watch.NewWatcher(paths)
	if err != nil {
	return nil, err
	}
	err = watcher.Start()
	if err != nil {
	return nil, err
	}

Although I guess it's nice to be able to construct a new watching without immediately starting it in some cases.

[ndeloof]

Start could indeed take place in constructor.
Start is not expected to be ran after watched was closed (channels are closed)

[thaJeztah]

Yea, I was looking because otherwise calling start multiple times would append a new listener to the global var, and start a new goroutine

compose/pkg/watch/watcher_darwin.go

Line 84 in e224264

numberOfWatches.Add(int64(len(d.stream.Paths)))

compose/pkg/watch/watcher_darwin.go

Line 90 in e224264

go d.loop()

So yeah, probably could be merged into NewWatcher then; looks like that requires an interface change 🤔

compose/pkg/watch/notify.go

Lines 38 to 40 in e224264

	type Notify interface {
	// Start watching the paths set at init time
	Start() error

For a more "organic" approach an alternative could also be to remove the Close function, and make Start return a "cancel" / "close" function, so that it's clear to the caller that it must be closed after, i.e. something like;

close, err := watcher.Start()
// ....
defer close()

Either way, that's an existing issue, and not something that's hit by the current code

[thaJeztah]

Perhaps the test should also run Start after this, and try Close again?

[thaJeztah]

LGTM to address the immediate issue