Bugfix: Correct minimum required bounds for serializing cstring and cstringUtf8 #538
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexbiehl
changed the title
sjakobi
reviewed
Aug 9, 2022
There was a problem hiding this comment.
I'm not very familiar with the Builder internals at this stage, but I think this fix is in line with the bufferFull haddocks and the other uses of bufferFull in this repo.
It would be great to get confirmation that this fixes yesodweb/wai#894 though.
clyring
approved these changes
Aug 9, 2022
|
Makes sense to me. @basvandijk any chance you can give this patch a spin please? |
Bodigrim
approved these changes
Aug 10, 2022
sjakobi
approved these changes
Aug 11, 2022
Bodigrim
pushed a commit
to Bodigrim/bytestring
that referenced
this pull request
Nov 25, 2022
angerman
added a commit
to angerman/nix-serve-ng
that referenced
this pull request
Dec 7, 2022
This fixes aristanetworks#19 by downgrading the compiler to 8.10 (I still don't think 9.0 should be used in production). bytestring < 0.11.4 (which as of today has not been released to https://hackage.haskell.org/package/bytestring), is broken, as per haskell/bytestring#538. This in turn shows up in yesodweb/wai#894, and ultimately causes aristanetworks#19.
|
Any chance we get a release out with this bug fix? |
clyring
added a commit
that referenced
this pull request
Dec 31, 2022
Bodigrim
pushed a commit
that referenced
this pull request
Jan 8, 2023
* Update changelog for v0.11.4.0 Co-authored-by: Matthew Craven <clyring@gmail.com> * Organize changelog entries for 0.11.4.0 * Re-phrase changelog entry for #538 Co-authored-by: Matthew Craven <clyring@gmail.com>
clyring
pushed a commit
that referenced
this pull request
Jan 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think this was an oversight in 155bf8a
The bufferFull signal takes the minimum number of required space to make progress. Instead, the defaultChunkSize was used. I can't prove it yet but I have a hunch this is causing yesodweb/wai#894. It looks like defaultChunkSize is 32k. A Warp buffer is something around 16k thus it will always hit the bufferFull signal when writing a cstring/cstringUtf8 requiring a bigger buffer which then makes Warp error out.
Pinging @bgamari as he is the author of the aforementioned patch and @basvandijk, maybe you can try out this fix and see whether it's fixing your issue?