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

Streaming multipart hangs for binary files above a certain threshold unless rechunking the body stream #2127

Closed
jgranstrom opened this issue Apr 29, 2023 · 8 comments · Fixed by #2162
Labels
💎 Bounty bug Something isn't working 💰 Rewarded

Comments

@jgranstrom
Copy link
Contributor

jgranstrom commented Apr 29, 2023

Describe the bug
Note: using 3.0.0-RC1+9-429c3938-SNAPSHOT which includes #2116

Sending a multipart binary file that is above a certain size, close to the StreamingForm buffer of 8192, I assume minus some header size, will cause the stream to hang indefinitely.

To Reproduce
Test server:

Server.serve(
    Http.collectZIO {
      case req @ Method.POST -> !! => (for {
        form <- req.body.asMultipartFormStream
        _ <- form.collectAll
      } yield Response.text("done")
        ).catchAll(e => ZIO.succeed(Response.text(e.toString)))
    }
  )

Sending an image with size 8155 is fine:
http --multipart localhost:8000 "file@./8155.png"
Sending an image with size 8156, or any size above that hangs indefinitely:
http --multipart localhost:8000 "file@./8156.png"

Creating the form manually with a custom buffer size, such as:
form <- StreamingForm(req.body.asStream, boundary, 8193)
Will now happily process one more byte, so 8156 will work, but anything above that will hang

Creating the form manually by first rechunking the body stream, such as:
form <- StreamingForm(req.body.asStream.rechunk(1024), boundary)
Seems to handle arbitrary sizes without hanging, although difficult to test exhaustively.

Expected behaviour
Any file size to be handled by streaming form without hanging. If rechunking the body stream is required for the form processing to work properly, it should probably be done by asMultipartFormStream

@jgranstrom jgranstrom added the bug Something isn't working label Apr 29, 2023
@jdegoes
Copy link
Member

jdegoes commented May 2, 2023

/bounty $100

@algora-pbc
Copy link

algora-pbc bot commented May 2, 2023

💎 $100.00 bounty created by jdegoes
👉 To claim this bounty, submit a pull request that includes the text /claim #2127 somewhere in its body
📝 Before proceeding, please make sure you can receive payouts in your country
💵 Payment arrives in your account 2-5 days after the bounty is rewarded
💯 You keep 100% of the bounty award
🙏 Thank you for contributing to zio/zio-http!

@algora-pbc
Copy link

algora-pbc bot commented May 5, 2023

💡 @ccerbusca submitted a pull request that claims the bounty. You can visit your org dashboard to reward.
👉 @ccerbusca: To receive payouts, sign up on Algora, link your Github account and connect with Stripe on your dashboard.

@jgranstrom
Copy link
Contributor Author

Since it doesn't reliably reproduce without a server, I used this test suite in ServerSpec which reproduces it consistently:

      suite("streaming form") {
        val boundary = Boundary("X-INSOMNIA-BOUNDARY")
        val app = Http.collectZIO[Request] { case req =>
          req.body.asMultipartFormStream
            .flatMap(f => f.collectAll)
            .map(form => Response.text(form.map("file").asInstanceOf[FormField.Binary].data.size.toString))
        }

        test("this succeeds"){
          val bytes = Chunk.fill[Byte](8000)(0)
          val form = Form(Chunk(FormField.Binary("file", bytes, MediaType.image.png)))
          val res = app.deploy.body.mapZIO(_.asString).run(
            headers = Headers(Header.ContentType(MediaType.multipart.`form-data`, Some(boundary))),
            body = Body.fromMultipartForm(form, boundary)
          )
          assertZIO(res)(equalTo("8000"))
        } +
          test("this times out") {
          val bytes = Chunk.fill[Byte](8200)(0)
          val form = Form(Chunk(FormField.Binary("file", bytes, MediaType.image.png)))
          val res = app.deploy.body.mapZIO(_.asString).run(
            headers = Headers(Header.ContentType(MediaType.multipart.`form-data`, Some(boundary))),
            body = Body.fromMultipartForm(form, boundary)
          )
          assertZIO(res)(equalTo("8200"))
        }
      },

@andrzejressel
Copy link
Contributor

andrzejressel commented May 5, 2023

Weird, it does repro for me in MultipartFormDataStreaming when I remove Server.Config.default.enableRequestStreaming.

EDIT: This is so weird, I'm starting to think Queues are to blame for this ...

@algora-pbc
Copy link

algora-pbc bot commented May 5, 2023

💡 @andrzejressel submitted a pull request that claims the bounty. You can visit your org dashboard to reward.
👉 @andrzejressel: To receive payouts, sign up on Algora, link your Github account and connect with Stripe on your dashboard.

@algora-pbc
Copy link

algora-pbc bot commented May 8, 2023

💡 @vigoo submitted a pull request that claims the bounty. You can visit your org dashboard to reward.

@algora-pbc
Copy link

algora-pbc bot commented May 8, 2023

🎉🎈 @vigoo has been awarded $100.00! 🎈🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💎 Bounty bug Something isn't working 💰 Rewarded
Projects
None yet
3 participants