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

Don't drain the body for non-streamed entities in EmberClient #6935

Merged
merged 1 commit into from Jan 22, 2023

Conversation

danicheg
Copy link
Member

No description provided.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Thanks, good catch :)

@valencik
Copy link
Member

Just to double check my understanding, this is a small performance optimization not a correctness / bug fix, right?

In the case of empty or strict entities we don't need to worry about draining all the bytes from the underlying stream because they are already all in memory, we don't have the risk of leaving some bytes from the socket unread?

@armanbilge
Copy link
Member

In the case of empty or strict entities we don't need to worry about draining all the bytes from the underlying stream because they are already all in memory

Exactly.

final case class Strict(bytes: ByteVector) extends Entity[Pure] {
def body: EntityBody[Pure] = Stream.chunk(Chunk.byteVector(bytes))

Comment on lines +50 to +51
case Entity.Default(body, _) =>
body.compile.drain.as(UnexpectedStatus(resp.status, req.method, req.uri))
Copy link
Member

Choose a reason for hiding this comment

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

The readability here will be super nice when/if #6305 merges 😎

@danicheg danicheg merged commit 3fc8d3b into http4s:main Jan 22, 2023
@danicheg danicheg deleted the ember-client branch January 22, 2023 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants