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

FS2 - Remove "elide empty chunks" #4351

Merged
merged 1 commit into from Feb 6, 2021
Merged

Conversation

diesalbla
Copy link
Contributor

@diesalbla diesalbla commented Feb 6, 2021

The FS2 library, by construction, guarantees that the uncons does not give, in the result, a non-empty chunk.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

When did this become true? FS2-3 or FS2-2? Is this something that could also be done on an earlier release?

@diesalbla
Copy link
Contributor Author

diesalbla commented Feb 6, 2021

When did this become true? FS2-3 or FS2-2? Is this something that could also be done on an earlier release?

I found about this invariant this week, but it would be interesting to know how far it extends.

If one would like to find out, here is what has to be done: checkout the release of fs2 being used, look for the Output class. This is the only subclass of Pull that contains a chunk, and it is on this Pull constructor that compilation stops when using the .uncons function. If that Output class is private, and the only call to its constructor includes the guard not to create it with empty chunk, as in this line, then the uncons should not be giving out an empty chunk.

@rossabaker
Copy link
Member

It appears that's true in fs2-2.5, in which case we could do the same all the way back to 0.21.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

This looks good after the comment is stripped. I'll probably try to cherry-pick it to 0.21, unless you want to reopen with the same logic against that.

The FS2 library, by construction, guarantees that the `uncons` will
never give, in the result, a non-empty chunk.
rossabaker pushed a commit that referenced this pull request Feb 6, 2021
The FS2 library, by construction, guarantees that the `uncons` will
never give, in the result, a non-empty chunk.
@rossabaker
Copy link
Member

It cherry-picked cleanly to series/0.21! 🎉

@rossabaker rossabaker closed this Feb 6, 2021
@rossabaker
Copy link
Member

Well, I should have run the tests instead of relying on reason and the compiler:

[error]   ! drain the epilogue with chunk size 3 (3 ms)
[error]    java.lang.RuntimeException: Chunk.empty.apply(0) (Chunk.scala:514)
[error] fs2.Chunk$EmptyChunk.apply(Chunk.scala:514)
[error] fs2.Chunk$EmptyChunk.apply(Chunk.scala:512)
[error] org.http4s.multipart.MultipartParser$.checkIfLast$1(MultipartParser.scala:312)

I'm going to revert the cherry-pick and merge the original.

@rossabaker rossabaker reopened this Feb 6, 2021
rossabaker added a commit that referenced this pull request Feb 6, 2021
@rossabaker rossabaker merged commit 1f5fa71 into http4s:main Feb 6, 2021
@diesalbla diesalbla deleted the no_empty_chunks branch April 7, 2022 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants