-
Notifications
You must be signed in to change notification settings - Fork 18.9k
introduce ImagePullResponse with helper method to manage JSONMessage stream decoding #50935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,10 @@ package jsonmessage | |
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "iter" | ||
| "strings" | ||
| "time" | ||
|
|
||
|
|
@@ -187,9 +189,32 @@ func (jm *JSONMessage) Display(out io.Writer, isTerminal bool) error { | |
| return nil | ||
| } | ||
|
|
||
| type JSONMessagesStream iter.Seq2[JSONMessage, error] | ||
|
|
||
| // DisplayJSONMessagesStream reads a JSON message stream from in, and writes | ||
| // each [JSONMessage] to out. It returns an error if an invalid JSONMessage | ||
| // is received, or if a JSONMessage containers a non-zero [JSONMessage.Error]. | ||
| // each [JSONMessage] to out. | ||
| // see DisplayJSONMessages for details | ||
| func DisplayJSONMessagesStream(in io.Reader, out io.Writer, terminalFd uintptr, isTerminal bool, auxCallback func(JSONMessage)) error { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. left this func for backward compatibility, maybe could ne removed ?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 NOT a blocker for this PR IMO, as it's some bike-shedding, but we should look at that before we release.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👇 copy/paste from Slack strictly speaking, 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;
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| var dec = json.NewDecoder(in) | ||
| var f JSONMessagesStream = func(yield func(JSONMessage, error) bool) { | ||
| for { | ||
| var jm JSONMessage | ||
| err := dec.Decode(&jm) | ||
| if errors.Is(err, io.EOF) { | ||
| break | ||
| } | ||
| if !yield(jm, err) { | ||
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return DisplayJSONMessages(f, out, terminalFd, isTerminal, auxCallback) | ||
| } | ||
|
|
||
| // DisplayJSONMessages writes each [JSONMessage] from stream to out. | ||
| // It returns an error if an invalid JSONMessage is received, or if | ||
| // a JSONMessage containers a non-zero [JSONMessage.Error]. | ||
| // | ||
| // Presentation of the JSONMessage depends on whether a terminal is attached, | ||
| // and on the terminal width. Progress bars ([JSONProgress]) are suppressed | ||
|
|
@@ -203,19 +228,12 @@ func (jm *JSONMessage) Display(out io.Writer, isTerminal bool) error { | |
| // - auxCallback allows handling the [JSONMessage.Aux] field. It is | ||
| // called if a JSONMessage contains an Aux field, in which case | ||
| // DisplayJSONMessagesStream does not present the JSONMessage. | ||
| func DisplayJSONMessagesStream(in io.Reader, out io.Writer, terminalFd uintptr, isTerminal bool, auxCallback func(JSONMessage)) error { | ||
| var ( | ||
| dec = json.NewDecoder(in) | ||
| ids = make(map[string]uint) | ||
| ) | ||
| func DisplayJSONMessages(messages JSONMessagesStream, out io.Writer, terminalFd uintptr, isTerminal bool, auxCallback func(JSONMessage)) error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs a docstring? |
||
| var ids = make(map[string]uint) | ||
|
|
||
| for { | ||
| for jm, err := range messages { | ||
| var diff uint | ||
| var jm JSONMessage | ||
| if err := dec.Decode(&jm); err != nil { | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a pointer receiver, otherwise the
r.rc = nilwill only be applied to the local copyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed. But then
ImagePullResponsecan't be used as anio.ReadCloser(need to return*ImagePullResponse). Let me test another approachThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, do we have a strict requirement for
ImagePullto not return a pointer?cc @thaJeztah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;Or perhaps an early return;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.rctonil?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's indeed risks for a race condition. Use of
sync.Onceprevents this as well