pkg/command: wrap `jsonmessage.DisplayJSONMessagesStream` with go context by Benehiko · Pull Request #5663 · docker/cli

Benehiko · 2024-12-02T14:39:40Z

Follow-up to #5639

Allows for the jsonmessage.DisplayJSONMessagesStream to
correctly return when the context is cancelled with the
appropriate error message, instead of just a nil error.

Follow-up to 30a73ff

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2025-01-13T14:56:27Z

Codecov Report

Attention: Patch coverage is 24.48980% with 37 lines in your changes missing coverage. Please review.

Project coverage is 59.42%. Comparing base (b462778) to head (91adb70).
Report is 34 commits behind head on master.

Additional details and impacted files

@@ Coverage Diff @@## master #5663 +/- ##
==========================================
- Coverage 59.51% 59.42% -0.10% 
==========================================
Files 346 347 +1 Lines 29376 29402 +26 ==========================================
- Hits 17483 17472 -11 - Misses 10923 10958 +35 - Partials 970 972 +2

vvoland · 2025-01-15T16:55:33Z

+// DisplayStream prints the JSON messages from the given reader to the given stream.
+//
+// It wraps the [jsonmessage.DisplayJSONMessagesToStream] function to make it
+// "context aware" and appropriately returns why the function was canceled.
+//
+// It returns an error if the context is canceled, but not if the input reader / stream is closed.
+func DisplayStream(ctx context.Context, in io.Reader, stream Stream, auxCallback func(JSONMessage)) error {
+	select {
+	case <-ctx.Done():
+ return ctx.Err()
+	default:
+ err := jsonmessage.DisplayJSONMessagesToStream(in, stream, auxCallback)
+ if err != nil {
+ return err
+ }
+ if ctx.Done() != nil {
+ return ctx.Err()
+ }
+	}
+	return nil
+}


We could drop the select to simplify this a bit
Suggested change
// DisplayStream prints the JSON messages from the given reader to the given stream.
//
// It wraps the [jsonmessage.DisplayJSONMessagesToStream] function to make it
// "context aware" and appropriately returns why the function was canceled.
//
// It returns an error if the context is canceled, but not if the input reader / stream is closed.
funcDisplayStream(ctx context.Context, in io.Reader, streamStream, auxCallbackfunc(JSONMessage)) error {
select {
case<-ctx.Done():
returnctx.Err()
default:
err:=jsonmessage.DisplayJSONMessagesToStream(in, stream, auxCallback)
iferr!=nil {
returnerr
}
ifctx.Done() !=nil {
returnctx.Err()
}
}
returnnil
}
// DisplayStream prints the JSON messages from the given reader to the given stream.
//
// It wraps the [jsonmessage.DisplayJSONMessagesToStream] function to make it
// "context aware" and appropriately returns why the function was canceled.
//
// It returns an error if the context is canceled, but not if the input reader / stream is closed.
funcDisplayStream(ctx context.Context, in io.Reader, streamStream, auxCallbackfunc(JSONMessage)) error {
ifctx.Err() !=nil {
returnctx.Err()
}
iferr:=jsonmessage.DisplayJSONMessagesToStream(in, stream, auxCallback); err!=nil {
returnerr
}
returnctx.Err()
}

We could also mostly support the context cancellation by wrapping the in io.Reader we pass into DisplayJSONMessagesToStream with something like:
typestopableReaderstruct { r io.Readerclosederror } func (c*stopableReader) Read(p []byte) (int, error) { if (c.closed!=nil) { return0, c.closed } returnc.r.Read(p) }
and then set closed = ctx.Err() after the context is done.

I think i'd prefer we be explicit about it instead of in the io.Reader. I'll remove the select

The io.Reader wrapper is a separate thing though.
With the current implementation the context cancellation isn't actually handled once the DisplayJSONMessagesToStream is called, the cancellation won't have any actual effect.
I pushed a small test to your branch to illustrate that.
The reader wrapper is trying to achieve that, that means this would effectively remove the need to make changes to the pkg/jsonmessage on moby side.

(sorry, late to comment; I saw the discussion, and was indeed wondering if closing the reader would make the jsonmessages properly cancel, as ISTR it was originally designed around some of that)

With the current implementation the context cancellation isn't actually handled once the DisplayJSONMessagesToStream is called, the cancellation won't have any actual effect.
yes, but the function was never designed for that. This PR is only trying to solve returning a context error instead of just a nil (when the reader is implicitly cancelled by context through say http.NewRequestWithContext).
The reader wrapper is trying to achieve that, that means this would effectively remove the need to make changes to the pkg/jsonmessage on moby side.
This PR's intent is to circumvent the moby side PR - or at least find an alternative without needing to refactor the function.
and was indeed wondering if closing the reader would make the jsonmessages properly cancel
~~Yes you are correct, it won't. The jsonmessages functions rely on the fact that io.EOF was returned to break the for loop.~~
EDIT: I meant to say that jsonmessages functions solely rely on io.EOF from the reader to exit, but that doesn't mean we will know why it was cancelled (the io.EOF reason). So closing the reader would exit the loop. https://github.com/moby/moby/blob/master/pkg/jsonmessage/jsonmessage.go#L237-L242

It will also exit on any Read other error and will return it:
https://github.com/moby/moby/blob/1153242d3a6c3635770db65d9233bb892ee1cbc3/pkg/jsonmessage/jsonmessage.go#L237-L243
If the Read inside Decode returns context.Canceled then it would also be returned from Decode.
That's what the stopableReader above does.
This PR's intent is to circumvent the moby side PR - or at least find an alternative without needing to refactor the function.
I get that, but why not make the DisplayStream function actually respect the cancellation if we can?
We control the reader passed so we can make it start returning error for subsequent Reads after the context is done.
With that stopableReader I posted above, we could just wait for ctx.Done() channel to be closed and set r.closed = ctx.Err() to make the reader start returning the context error.

i see, alright I'll look into this then

As I mentioned in #5645 (comment) and #5645 (comment), I don't think adding a context to jsonmessage functions is the best approach – it would be simpler to let those continue only caring about whether the read fails or not to know when to stop instead of having to care about both things.
@vvoland's suggestion allows addressing the issue without needless changes to jsonmessage.

thaJeztah · 2025-01-17T11:34:06Z

+// "context aware" and appropriately returns why the function was canceled.
+//
+// It returns an error if the context is canceled, but not if the input reader / stream is closed.
+func DisplayStream(ctx context.Context, in io.Reader, stream Stream, auxCallback func(JSONMessage)) error {


One other thing consider is to add 2 separate functions; one that takes an auxCallBack and one without; looking at the CLI code, I see that we need the auxCallBack in only 2 places;
cli/command/container/create.go: return jsonmessage.DisplayJSONMessagesToStream(responseBody, out, nil) cli/command/image/import.go: return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil) cli/command/image/load.go: return jsonmessage.DisplayJSONMessagesToStream(response.Body, dockerCli.Out(), nil) cli/command/image/push.go: err = jsonmessage.DisplayJSONMessagesToStream(responseBody, streams.NewOut(io.Discard), handleAux(dockerCli)) cli/command/image/push.go: return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), handleAux(dockerCli)) cli/command/image/trust.go: if err := jsonmessage.DisplayJSONMessagesToStream(in, ioStreams.Out(), nil); err != nil { cli/command/image/trust.go: if err := jsonmessage.DisplayJSONMessagesToStream(in, ioStreams.Out(), handleTarget); err != nil { cli/command/image/trust.go: return jsonmessage.DisplayJSONMessagesToStream(responseBody, out, nil) cli/command/plugin/install.go: if err := jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil); err != nil { cli/command/plugin/push.go: return jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil) cli/command/plugin/upgrade.go: if err := jsonmessage.DisplayJSONMessagesToStream(responseBody, dockerCli.Out(), nil); err != nil { cli/command/service/helpers.go: err := jsonmessage.DisplayJSONMessagesToStream(pipeReader, dockerCli.Out(), nil) cli/command/swarm/ca.go: err := jsonmessage.DisplayJSONMessagesToStream(pipeReader, dockerCli.Out(), nil)
Alternatively, we could consider (for a expandable UX) having it accept a DisplayOptions{} struct (or fancy functional DisplayOptions()) , e.g.
err:=DisplayWithOptions(ctx, in, stream, Options{ AuxCallback: myCallback, })

another is to make Display a variadic function like so:
typeOptionsfunc(*options) typeoptionsstruct { AuxCallbackfunc(JSONMessage) } funcWithAuxCallback(cbfunc(JSONMessage)) Options { returnfunc(o*options) { o.AuxCallback=cb } } funcDisplay(ctx context.Context, in io.Reader, streamStream, opts...Options) error { }
Then when you use it, you don't even need to specify the last parameter in most cases.
err:=jsonstream.Display(ctx, reader, out) // orerr:=jsonstream.Display(ctx, reader, out, jsonstream.WithAuxCallback(func(j jsonstream.JSONMessage){}))

thaJeztah · 2025-01-17T11:35:15Z

@@ -0,0 +1,32 @@
+package jsonmessage


If we could name this package jsonstream it could help disambiguate with the jsonmessage package, and we could name the function Display() as it would be called as jsonstream.Display()

thaJeztah · 2025-01-17T11:41:17Z

+type (
+	Stream = jsonmessage.Stream
+	JSONMessage = jsonmessage.JSONMessage
+	JSONError = jsonmessage.JSONError


This is still an internal package, so not too problematic, but it looks like JSONError is literally used in a single place in the CLI. We may want to have a look if we want to keep it as an exported type, or if there's alternatives to reduce the "public" (but right now, still "internal") API;
cli/cli/command/image/build.go
Lines 366 to 375 in f4a68da
ifjerr, ok:=err.(*jsonmessage.JSONError); ok {
// If no error code is set, default to 1
ifjerr.Code==0 {
jerr.Code=1
}
ifoptions.quiet {
fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff)
}
return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code}
}

The idea behind this was to keep all the jsonmessage code in a singular place inside an internal package inside the CLI. Maybe i'm being a bit excessive, but it felt cleaner

…text Allows for the `jsonmessage.DisplayJSONMessagesStream` function to correctly return when the context is cancelled with the appropriate reason (`ctx.Error()`) instead of just a nil error. Follow-up to docker@30a73ff Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Co-authored-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland

LGTM

Benehiko force-pushed the ctx-jsonmessage branch 2 times, most recently from c21438f to 4884112Compare December 2, 2024 14:42

Benehiko force-pushed the ctx-jsonmessage branch from 4884112 to 51e16eeCompare January 13, 2025 14:54

Benehiko changed the title ~~pkg/command: pass context along to DisplayJSONMessagesStream~~pkg/command: wrap DisplayJSONMessagesToStream and DisplayJSONMessagesStreamJan 13, 2025

Benehiko force-pushed the ctx-jsonmessage branch 3 times, most recently from d55ee6d to bfd3131Compare January 13, 2025 15:30

Benehiko commented Jan 15, 2025
View reviewed changes

Comment threadcli/internal/jsonmessage/display.go Outdated

Benehiko requested review from thaJeztah and vvoland January 15, 2025 12:50

vvoland reviewed Jan 15, 2025
View reviewed changes

Comment threadcli/command/image/build.go Outdated

Benehiko force-pushed the ctx-jsonmessage branch from bfd3131 to 0298524Compare January 15, 2025 16:13

vvoland reviewed Jan 15, 2025
View reviewed changes

Benehiko force-pushed the ctx-jsonmessage branch from 0298524 to 9d31dfbCompare January 16, 2025 09:45

Benehiko changed the title ~~pkg/command: wrap DisplayJSONMessagesToStream and DisplayJSONMessagesStream~~pkg/command: wrap jsonmessage.DisplayJSONMessagesStream with go contextJan 16, 2025

Benehiko requested a review from vvoland January 16, 2025 10:06

thaJeztah reviewed Jan 17, 2025
View reviewed changes

vvoland reviewed Jan 20, 2025
View reviewed changes

Comment threadcli/internal/jsonstream/display.go Outdated

Benehiko force-pushed the ctx-jsonmessage branch from 6fe1f2e to 933ed57Compare January 20, 2025 10:51

Benehiko requested review from thaJeztah and vvoland January 20, 2025 13:39

Benehiko added the status/2-code-review label Jan 20, 2025

Benehiko added this to the 28.0.0 milestone Jan 20, 2025

vvoland reviewed Jan 20, 2025
View reviewed changes

Comment threadcli/internal/jsonstream/display.go Outdated

Benehiko force-pushed the ctx-jsonmessage branch from 933ed57 to 91adb70Compare January 20, 2025 15:36

vvoland approved these changes Jan 21, 2025
View reviewed changes

Benehiko merged commit 41c2786 into docker:masterJan 21, 2025

Benehiko mentioned this pull request Jan 29, 2025
pkg/jsonmessage: should return early on context cancellation moby/moby#49016
Closed

-// DisplayStream prints the JSON messages from the given reader to the given stream.
-//
-// It wraps the [jsonmessage.DisplayJSONMessagesToStream] function to make it
-// "context aware" and appropriately returns why the function was canceled.
-//
-// It returns an error if the context is canceled, but not if the input reader / stream is closed.
-funcDisplayStream(ctx context.Context, in io.Reader, streamStream, auxCallbackfunc(JSONMessage)) error {
-select {
-case<-ctx.Done():
-returnctx.Err()
-default:
-err:=jsonmessage.DisplayJSONMessagesToStream(in, stream, auxCallback)
-iferr!=nil {
-returnerr
- }
-ifctx.Done() !=nil {
-returnctx.Err()
- }
-	}
-returnnil
-}
+// DisplayStream prints the JSON messages from the given reader to the given stream.
+//
+// It wraps the [jsonmessage.DisplayJSONMessagesToStream] function to make it
+// "context aware" and appropriately returns why the function was canceled.
+//
+// It returns an error if the context is canceled, but not if the input reader / stream is closed.
+funcDisplayStream(ctx context.Context, in io.Reader, streamStream, auxCallbackfunc(JSONMessage)) error {
+ifctx.Err() !=nil {
+returnctx.Err()
+	}
+iferr:=jsonmessage.DisplayJSONMessagesToStream(in, stream, auxCallback); err!=nil {
+returnerr
+ }
+returnctx.Err()
+}

	ifjerr, ok:=err.(*jsonmessage.JSONError); ok {
	// If no error code is set, default to 1
	ifjerr.Code==0 {
	jerr.Code=1
	}
	ifoptions.quiet {
	fmt.Fprintf(dockerCli.Err(), "%s%s", progBuff, buildBuff)
	}
	return cli.StatusError{Status: jerr.Message, StatusCode: jerr.Code}
	}

Uh oh!

Conversation

Benehiko commented Dec 2, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jan 13, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenehikoJan 16, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenehikoJan 17, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Benehiko commented Dec 2, 2024•
edited
Loading

codecov-commenter commented Jan 13, 2025•
edited
Loading

BenehikoJan 16, 2025•
edited
Loading

BenehikoJan 17, 2025•
edited
Loading