[#2112] progress.Controller should own the progress.Writer to prevent leaks #2203

coryb · 2021-06-25T16:38:16Z

To prevent leaks with Close not being called on progress.Writer there is an expectation that everywhere we Write to a progress.Writer we should also be calling Close when the writing is complete. I looked through the code and currently the only occurrence of Write without Close was in the progress.Controller. Moving the lifecycle management of the progress.Writer into the reference-counted implementation of the Controller fixes the leak.

tonistiigi · 2021-06-25T17:40:02Z

source/containerimage/pull.go

-			progressController := &controller.Controller{
-				Writer: pw,
-			}
+			progressController := &controller.Controller{}


I think this loses the correct context for the progress. Eg. lazy blob pulls happening later should still be associated with the image where they are coming from. I think we need to read out the progress writer from context in here but not initialize it yet. Only read out constructor/callback.

It is confusing with the multiple contexts involved here, I was hoping the ctx passed into the Start would be the right one, but sounds like not. If my understanding is correct, I think we have two options:

Capture ctx at the time we create the &controller.Controller{}, perhaps store a progressCtx in the struct for later use? For this I would just add func NewController(context.Context) progress.Controller

Split progress.FromContext into two parts, one to just return the progress stored in ctx.Value() so we can stash it in the Controller, and then another that does the newWriter. If we go this route I think I would rename progress.FromContext to progress.NewFromContext. Then add a new progress.FromContext that would just load progress from ctx.Value. Then rename newWriter(Writer) to NewWriter(writer) so it can be used outside the package.

Thoughts?

I went with option 2) and updated the PR, if you disagree should be easy enough to change.

sipsma

LGTM

tonistiigi · 2021-06-28T03:30:00Z

util/progress/progress.go

+// NewWriter returns a new progress writer based on the Writer argument.
+// It is the callers responsibility to call Close on the Writer to prevent
+// resource leaks.
+func NewWriter(w Writer, opts ...WriterOption) Writer {


Generally ok with this approach but this public API makes little sense. Both New and From return writer, NewWriter takes writer as a parameter. I think better would be if FromContext returns ProgressFactory, a function that returns a new writer when called. NewWriter is not public.

Okay, updated. I went with WriterFactory to avoid the stutter with progress.ProgressFactory.

aaronlehmann · 2021-06-28T16:58:53Z

util/progress/progress.go

+// responsibility to Close the returned Writer to avoid resource leaks.
+type WriterFactory func() (Writer, bool, context.Context)
+
+// FromContext returns a ProgressFactory to generate new progress writers


Suggested change

// FromContext returns a ProgressFactory to generate new progress writers

// FromContext returns a WriterFactory to generate new progress writers

tonistiigi · 2021-06-28T17:16:18Z

util/progress/progress.go

+		for _, o := range opts {
+			o(pw)
+		}
+		ctx = context.WithValue(ctx, contextKey, pw)


~~I think this should return (WriterFactory, bool, context.Context)~~. It is wrong for the callback to return context that was created by another stack. That goroutine is likely already completed and original context has been canceled. If you have a case where you need new context when newWriter is called then the callback itself should take a context and new writer is added to that context, not to the one that was passed into FromContext.

I see that newWriter() result is added to the context. If that is the requirement and there isn't another way to reorganize this that means that my second proposal makes more sense. For the default case, you can add back NewFromContext helper so that caller does not need to pass context twice when from&new happen together.

Okay, makes sense. Update the WriterFactory now takes a ctx and added NewFromContext wrapper to automatically call the WriterFactory with the same ctx passed in.

tonistiigi · 2021-06-28T18:53:41Z

Looks like I broke this with #2195 merge

#17 [build 2/2] RUN --mount=target=. --mount=type=cache,target=/root/.cache   --mount=target=/go/pkg/mod,type=cache   --mount=source=/tmp/.ldflags,target=/tmp/.ldflags,from=version   CGO_ENABLED=0 xx-go build -o /dockerfile-frontend -ldflags "-d $(cat /tmp/.ldflags)" -tags "dfrunsecurity dfrunnetwork dfheredoc netgo static_build osusergo" ./frontend/dockerfile/cmd/dockerfile-frontend &&   xx-verify --static /dockerfile-frontend
#17 sha256:88dadf8da57dee2e60b61c4163e6c386679c0242d71b65d52f07e5d9ba778010
#17 0.323 # github.com/moby/buildkit/util/flightcontrol
#17 0.323 util/flightcontrol/flightcontrol.go:139:13: assignment mismatch: 3 variables but progress.FromContext returns 1 values

Rebase should fix it.

coryb · 2021-06-28T18:54:26Z

Test failing with an odd error:

#16 0.054 env: can't execute 'bash': No such file or directory
#16 0.055 /bin/sh: git: not found
#16 0.055 /bin/sh: git: not found

https://github.com/moby/buildkit/pull/2203/checks?check_run_id=2934987969#step:7:3822

…vent leaks Signed-off-by: Cory Bennett <cbennett@netflix.com>

this allows progress.Controller to manage the writer lifecycle Signed-off-by: Cory Bennett <cbennett@netflix.com>

tonistiigi · 2021-06-28T19:01:21Z

@coryb created #2206 but shouldn't be what caused the failure

coryb · 2021-06-28T19:48:40Z

Rebased and tests finally pass 🎉

tonistiigi reviewed Jun 25, 2021

View reviewed changes

tonistiigi requested a review from sipsma June 25, 2021 17:40

coryb force-pushed the issue-2112 branch from 82c0eb8 to f1313df Compare June 25, 2021 20:06

sipsma approved these changes Jun 28, 2021

View reviewed changes

tonistiigi reviewed Jun 28, 2021

View reviewed changes

coryb force-pushed the issue-2112 branch from f1313df to 97da31b Compare June 28, 2021 16:53

aaronlehmann reviewed Jun 28, 2021

View reviewed changes

tonistiigi reviewed Jun 28, 2021

View reviewed changes

coryb force-pushed the issue-2112 branch from 97da31b to 0601d52 Compare June 28, 2021 18:34

tonistiigi approved these changes Jun 28, 2021

View reviewed changes

coryb added 2 commits June 28, 2021 18:56

[moby#2112] progress.Controller should own the progress.Writer to pre…

1da8b67

…vent leaks Signed-off-by: Cory Bennett <cbennett@netflix.com>

[moby#2112] progress.FromContext returns a writer factory

b1d441b

this allows progress.Controller to manage the writer lifecycle Signed-off-by: Cory Bennett <cbennett@netflix.com>

coryb force-pushed the issue-2112 branch from 0601d52 to b1d441b Compare June 28, 2021 18:58

tonistiigi approved these changes Jun 28, 2021

View reviewed changes

coryb merged commit f4fcba5 into moby:master Jun 28, 2021

tonistiigi mentioned this pull request Jul 13, 2021

[0.9] blob download progress broken #2248

Closed

crazy-max mentioned this pull request Oct 17, 2022

[v0.8 backport] log clipping fixes #3186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#2112] progress.Controller should own the progress.Writer to prevent leaks #2203

[#2112] progress.Controller should own the progress.Writer to prevent leaks #2203

coryb commented Jun 25, 2021

tonistiigi Jun 25, 2021

coryb Jun 25, 2021

coryb Jun 25, 2021

sipsma left a comment

tonistiigi Jun 28, 2021

coryb Jun 28, 2021

aaronlehmann Jun 28, 2021

tonistiigi Jun 28, 2021 •

edited

Loading

tonistiigi Jun 28, 2021

coryb Jun 28, 2021

tonistiigi commented Jun 28, 2021 •

edited

Loading

coryb commented Jun 28, 2021

tonistiigi commented Jun 28, 2021

coryb commented Jun 28, 2021

	// FromContext returns a ProgressFactory to generate new progress writers
	// FromContext returns a WriterFactory to generate new progress writers

[#2112] progress.Controller should own the progress.Writer to prevent leaks #2203

[#2112] progress.Controller should own the progress.Writer to prevent leaks #2203

Conversation

coryb commented Jun 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sipsma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi Jun 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonistiigi commented Jun 28, 2021 • edited Loading

coryb commented Jun 28, 2021

tonistiigi commented Jun 28, 2021

coryb commented Jun 28, 2021

tonistiigi Jun 28, 2021 •

edited

Loading

tonistiigi commented Jun 28, 2021 •

edited

Loading