Skip to content
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

newline between JSON messages #931

Merged
merged 3 commits into from
May 10, 2019

Conversation

jhump
Copy link
Contributor

@jhump jhump commented May 9, 2019

This updates JSONPb so that its encoder function delimits output messages with a newline, just like Encoder in "encoding/json" does.

Copy link
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this interact with JSONPb.Delimiter?

runtime/marshal_jsonpb.go Show resolved Hide resolved
@jhump
Copy link
Contributor Author

jhump commented May 9, 2019

How does this interact with JSONPb.Delimiter?

It needs to use the same text, so that the NewEncoder ends up delimiting messages the same way that server-stream output will delimit them. (Server stream output doesn't use NewEncode. It instead directly calls Marshal and then writes Delimiter, if the marshaler has such a method.)

I've updated to make sure these are consistent (per what I think you were recommending). PTAL.

@codecov-io
Copy link

Codecov Report

Merging #931 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #931      +/-   ##
==========================================
- Coverage    53.5%   53.49%   -0.01%     
==========================================
  Files          40       40              
  Lines        3957     3961       +4     
==========================================
+ Hits         2117     2119       +2     
- Misses       1642     1643       +1     
- Partials      198      199       +1
Impacted Files Coverage Δ
runtime/marshal_jsonpb.go 68.5% <60%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15c7b2e...1b30b06. Read the comment docs.

@johanbrandhorst johanbrandhorst merged commit e178b56 into grpc-ecosystem:master May 10, 2019
@johanbrandhorst
Copy link
Collaborator

Thanks for your contribution!

}
// mimic json.Encoder by adding a newline (makes output
// easier to read when it contains multiple encoded items)
_, err := w.Write(j.Delimiter())
Copy link

@socketpair socketpair Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In etcd this may be written to different HTTP chunk, right? As far as I know, the protocol must guarantee that 1 chunk = 1 message (JSON).

I don't understand go, but I have the code that triggers that on high load. I mean, sometimes newline is seen as a separate HTTP chunk.

moreover, comment here state that newline is only for easy debugging. May I rely on it safely ? if yes, please update the documentation. If no, please state it. And also please write something about message=chunk atomicity.

Copy link

@socketpair socketpair Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moreover. What also stops from gluing two messages in one HTTP chunk?

What I propose: is to document that every message is separated strictly by one newline. In that case, there is possible to write stable and simple stream parsing that does not rely on HTTP chunk boundary, and which also does not use streaming JSON parser to detect end of a message. If you accept that, you should document that users must not rely on chunk boundaries in order to split stream to messages.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please raise a separate issue detailing your concerns? I don't think this change covers the behaviour you're asking about.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adasari pushed a commit to adasari/grpc-gateway that referenced this pull request Apr 9, 2020
* newline between JSON messages from JSONPb.Encode

* use j.Delimiter()

* fix jsonpb tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants