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

Multipart default improvements, mixed file/in memory streaming decoder #1865

Merged
merged 11 commits into from May 25, 2018

Conversation

Projects
None yet
3 participants
@jmcardon
Member

jmcardon commented May 16, 2018

Continuation of #1862

@rossabaker

Should we add to the scaladoc that if you don't consume the parts, you may leak temp files?

@jmcardon

This comment has been minimized.

Member

jmcardon commented May 16, 2018

I can add it, sure.

@jmcardon

This comment has been minimized.

Member

jmcardon commented May 18, 2018

Do we want max size per part? This is the only thing that we don't have.

Alternatively, we can pass it a configuration object instead of relying purely on args.

@rossabaker

This comment has been minimized.

Member

rossabaker commented May 18, 2018

Configuration arguments are good if we avoid case classes, so more arguments (with defaults) can be added later in a compatible way.

I don't know that we care that much about max size per part? If we limit the total size of the body, and we limit the number of parts so we don't get flooded with 1,000,000 single-byte temp files (if the threshold is turned down), then I'm not sure what we're protecting ourselves from. Though I'd take a peak at other common libraries, as we can learn a lot from history of more mature implementations.

@jmcardon

This comment has been minimized.

Member

jmcardon commented May 18, 2018

That's fair. I think we can just ask people to limit entire bodies if they're worried about a threshold instead.

@jmcardon

This comment has been minimized.

Member

jmcardon commented May 25, 2018

for the record the last commit is just a semantic change since I had a redundant assignment for some reason. I think val bytes = chunk used be like val bytes = doStuffWithChunk(chunk) or something and then I took that out.

@rossabaker rossabaker merged commit 8ea023b into http4s:release-0.18.x May 25, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment