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

Copy HttpContent into array to avoid buffer being released too early #4194

Merged
merged 5 commits into from
Dec 3, 2024

Conversation

sbrunk
Copy link
Contributor

@sbrunk sbrunk commented Dec 2, 2024

We've run into issues when decoding large JSON bodies using the netty cats backend. For some requests, decoding failed, due to parts of the input JSON missing.

The issue doesn't occur every time and is not deterministic, but we were able to track it down to this PR: #3489.

We believe that the issue is caused by the buffer sometimes being released to early, as the fs2 Chunk doesn't copy it and httpContent.release() sometimes trigger a deallocation by netty before the buffer is read into an array later. Our proposed fix is to explicitly copy the buffer into an array and only then call release().

We've added a new performance test for large JSON bodies that reliably shows the error. You should be able to compare the difference yourself by running:

perfTests/runMain sttp.tapir.perf.apis.ServerRunner netty.cats.Tapir
perfTests/Gatling/testOnly sttp.tapir.perf.PostLongJsonSimulation

Before:
image
After:
image

@pektinasen

@sbrunk
Copy link
Contributor Author

sbrunk commented Dec 2, 2024

Here's also a standalone Scala CLI script as Gist to demonstrate the issue.

@adamw
Copy link
Member

adamw commented Dec 3, 2024

Thanks for the investigation and the fix! I turned this into a regular test, since we're talking about correctness, not only performance here. I suspected that the ZIO integration might have a similar problem, but it turns out their Chunk.fromByteBuffer already copies data into an array. We'll also see how the other backends fare with the new test ;)

sbrunk added a commit to sbrunk/tapir that referenced this pull request Dec 3, 2024
… into a release

Contains only the fix based on 1.11.9
@adamw adamw merged commit 38d0339 into softwaremill:master Dec 3, 2024
28 checks passed
@sbrunk
Copy link
Contributor Author

sbrunk commented Dec 3, 2024

Thanks for getting it to the finish line!

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.

2 participants