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

Refactor request body handling to fix issue #147 #206

Closed
wants to merge 9 commits into from

Conversation

thomasjm
Copy link

Issue #147 happens because of the semantics of Network.WAI's requestBody function, which is an IO action that you must call repeatedly to get all the chunks of the body. Previously, Scotty's route handler code made the mistake of evaluating the whole body in Route.hs:mkEnv, which gets called on every matched route. As a result, if you match a route which then calls next, the body will be consumed already and will be unavailable to future routes.

The solution in this PR is to recognize that we can't represent the routes in ScottyState simply as a [Middleware m], because evaluation of an earlier Middleware can screw up the state for future ones. Instead, we need an extra top-level piece of state, which I've called the BodyInfo. This state will keep track of all the body chunks that we've read and provide them to the route-processing code, while preserving the semantics of things like bodyReader.

A BodyInfo is created in scottyAppT at the beginning of request processing, and has 3 things:

  1. An MVar containing a list of all the body chunks read so far, plus a flag indicating whether we've reached the end of the stream yet
  2. An MVar containing an index into this stream, used to implement bodyReader, and
  3. The actual requestBody function which can be called to get more chunks

The key part of having two MVars is that we can "clone" the BodyInfo to create a copy where the index is reset to 0, but the chunk cache is the same. Passing a cloned BodyInfo into each matched route allows them each to start from the first chunk if they call bodyReader.

Please let me know what you think! One TODO is to add some tests that bodyReader actually works properly, does anyone know how I can test a chunked request?

@coveralls
Copy link

coveralls commented Jul 28, 2017

Coverage Status

Coverage increased (+3.2%) to 60.472% when pulling 2fc0e0c on thomasjm:master into 3083cf5 on scotty-web:master.

@@ -0,0 +1,66 @@
# This file was automatically generated by 'stack init'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove stack.yaml? I think it's better to avoid having stack.yaml in a library repository, since it easily gets outdated and makes it harder for devs to test in multiple GHC versions.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I stopped using Scotty years ago. You can feel free to close this PR or use any parts that are still useful.

@thomasjm thomasjm closed this Jun 13, 2022
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.

3 participants