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

Factor out chunked reader from Client::request_events #379

Closed
clux opened this issue Jan 9, 2021 · 2 comments · Fixed by #394
Closed

Factor out chunked reader from Client::request_events #379

clux opened this issue Jan 9, 2021 · 2 comments · Fixed by #394
Labels
client http issues with the client

Comments

@clux
Copy link
Member

clux commented Jan 9, 2021

Currently the chunk buffering is inlined into the request_events call.
This makes the function huge, hard to follow, and hard to test.

It would be better if there was a more cleanly separated Reader like struct that dealt with buffering and error handling of un-closed chunks specifically.

Not a big issue, but it makes that code a bit awkward.

@clux clux added the client http issues with the client label Jan 9, 2021
@kazk
Copy link
Member

kazk commented Jan 9, 2021

Yeah, I was going to ask about this. Is the response stream of line delimited JSON? Can we use tokio_util::codec::FramedRead with tokio_util::codec::LinesCodec?

@clux
Copy link
Member Author

clux commented Jan 9, 2021

Ooh, yeah those look promising. The body of the fn looks very much like an impl Decoder at the very least.

Yes, it's newline delimited JSON. You have to test it after each chunk to see if you have a complete object since the objects can exceed MTU.

@clux clux linked a pull request Jan 9, 2021 that will close this issue
@kazk kazk linked a pull request Feb 7, 2021 that will close this issue
@clux clux closed this as completed in #394 Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client http issues with the client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants