I think we should make it more explicit -

In future, I think we should have a better public interface than JSONMessage and we wouldn't need to change the signaure of Messages then.

Also, perhaps worth passing a ctx so there's a way to cancel this without consuming all messages?

added context as parameter in JSONMessages so stream processing is stopped and reader closed if context is cancelled + test case

left this func for backward compatibility, maybe could ne removed ?

Just a "note to self", as @vvoland 's comment reminded me of that.

I recall when we moved this package that we also considered cleaning up the API; there was a change in the CLI to improve some bits (passing context etc), but also to properly take advantage of the package name, so changing these to jsonmessage.Stream and jsonmessage.Display (or jsonmessage.DisplayStream).

https://github.com/docker/cli/blob/v28.5.0/internal/jsonstream/display.go

NOT a blocker for this PR IMO, as it's some bike-shedding, but we should look at that before we release.

I was wondering if this is safe to change DisplayJSONMessagesStream signature to drop support for io.Reader. If this is fine, then I can also adopt a shorted name

👇 copy/paste from Slack

strictly speaking, moby/client is a completely new module; we have no requirement to carry anything from the old docker/docker/client module, as long as we can provide a replacement for users of it, which sometimes requires a bit of digging (I tend to use https://grep.app to get a quick "guess" what's used, and where, and if it's relevant) to see if it can reasonably be swapped.

The whole json-message was a massacre; it really had to be reverse-engineered to understand "what was the meaning here in the first place?"; fun things I discovered was that (IIRC) there were no daemon-side types corresponding with it! It just happened to be a type that could unmarshal some random other types produced by the daemon because some of the fields matched (basically it "cherry-picked" some fields produced by build, some other fields for events, and yet some other fields for image pull / push streams).

I jotted down some things in #50575 - but even with changes I made in the API, it's possible that definitions are in the wrong place, or ... shouldn't be there;

• https://github.com/moby/moby/tree/master/api/types/jsonstream
• https://github.com/moby/moby/tree/master/api/pkg/progress
• https://github.com/moby/moby/tree/master/api/pkg/streamformatter
• also slightly related; https://github.com/moby/moby/tree/master/api/pkg/stdcopy

• opened a quick ticket to look at these functions after this PR is merged; client: look if we need DisplayJSONMessages and DisplayJSONMessagesStream #51145

Also to be clear w.r.t. my other comment; this SGTM, because we're not in the jsonmessage package here, so it's ok to be explicit.

We should probably make it a concrete field and then implementing the io.ReadCloser for the ImagePullResponse.

This will also allow us to wrap Close method so we don't close the underlying reader twice in cases like:

out, err := ImagePull(...)
...

defer out.Close()

for i in out.JSONMessages() {
...
}
...

done

This needs a pointer receiver, otherwise the r.rc = nil will only be applied to the local copy

indeed. But then ImagePullResponse can't be used as an io.ReadCloser (need to return *ImagePullResponse). Let me test another approach

Hmm, do we have a strict requirement for ImagePull to not return a pointer?

cc @thaJeztah

I haven't looked in depth at the problem, but w.r.t. pointer vs not; I don't think we have a strict requirement; In fact for some cases, I like pointers, because they allow return nil, err, and callers should never consume returned values on error 😄. See #51076 (comment)

Just a quick comment; I'd like to avoid named error output variables; at least if they're named err - in this case it's a very small function, but we've had cases where errors were not handled correctly, so;

// Close implements io.ReadCloser
func (r *ImagePullResponse) Close() error {
	if r.rc != nil {
		err := r.rc.Close()
		r.rc = nil
		return err
	}
	return nil
}

Or perhaps an early return;

// Close implements io.ReadCloser
func (r *ImagePullResponse) Close() error {
	if r.rc == nil {
		return nil
	}
	err := r.rc.Close()
	r.rc = nil
	return err
}

Looking at that; could there be a race happening in the above? (can close be called by anything concurrently and panic if one wins and sets r.rc to nil?)

there's indeed risks for a race condition. Use of sync.Once prevents this as well

Quick comment before I forget (havent reviewed the latest iteration in depth);

do we want this one exported, or consider it internal to the client?

let me make this one internal, we could export in the future if there's some usages for it - better safe than sorry

This needs a docstring?

It'd be worth saying it closes the ImagePullResponse when the context is cancelled or the stream ends?

Probably best not to use a named result & naked return, and just var err error in the body.

Oh - this was wrong before, but I don't think the comment will be picked up by godoc as the function name doesn't match. Maybe worth fixing, now it has more info?