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

json_nl: Emit newlines immediately, rather than waiting for next stream item #21

Closed
sartak opened this issue Jul 27, 2023 · 3 comments · Fixed by #22
Closed

json_nl: Emit newlines immediately, rather than waiting for next stream item #21

sartak opened this issue Jul 27, 2023 · 3 comments · Fixed by #22
Labels
bug Something isn't working

Comments

@sartak
Copy link

sartak commented Jul 27, 2023

Hi, thanks for axum-streams-rs. I noticed that the newline-delimited JSON format emits the newline separator before each item (except for the very first). This makes it harder to use than if every object were immediately followed by a newline.

On the consumer side, the natural, easy way to handle newline-delimited JSON (in general) is to buffer the response until you see a newline, then pass that off to JSON.parse. If you do that with the current implementation of this library, you'll always be delayed by the latency of the next message, and you'd potentially miss the very last message (unless you handle the edge case of a valid JSON object with no newline on stream close). So you'd need a more sophisticated response, such as trying to parse the buffer as JSON each time you get more content on the TCP stream, which is more expensive and complicated.

Relatedly, https://github.com/ndjson/ndjson-spec#31-serialization states "Each JSON text MUST […] be written to the stream followed by the newline character \n".

Please consider changing this code to emit the newline immediately after each object, unconditionally.

if index != 0 {
buf.write_all(JSON_NL_SEP_BYTES).unwrap();
}
match serde_json::to_writer(&mut buf, &obj) {
Ok(_) => Ok(buf.into_inner().freeze()),
Err(e) => Err(axum::Error::new(e)),
}

@abdolence
Copy link
Owner

Hey,

Interesting finding. So the problem actually happens only if we have one element to emit, so in this case it will be emitted without \n. Let me look into this.

@abdolence abdolence added the bug Something isn't working label Jul 27, 2023
@abdolence abdolence linked a pull request Jul 27, 2023 that will close this issue
@abdolence
Copy link
Owner

abdolence commented Jul 27, 2023

This should work now in https://github.com/abdolence/axum-streams-rs/releases/tag/v0.9.1

Thanks for reporting it!

@sartak
Copy link
Author

sartak commented Jul 28, 2023

Thank you, works great! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants