Fix ReadBodyFromRequestAsync disposing ctx.Request.Body (#615)

* add leaveOpen=true to ReadBodyFromRequestAsync, so the ctx.Request.Body is not disposed

* add helper test function getReqBody, and use the leaveOpen = true at the getBody

* add more tests to assert that the body is being disposed correctly when using ReadBodyFromRequestAsync, and add a new ReadBodyFromRequestAsync function that let's the user define the value of the leaveOpen parameter (useful for tests)

Author: 64J0

src/Giraffe/HttpContextExtensions.fs

@@ -203,7 +203,22 @@ type HttpContextExtensions() =
     [<Extension>]
     static member ReadBodyFromRequestAsync(ctx: HttpContext) =
         task {
-            use reader = new StreamReader(ctx.Request.Body, Encoding.UTF8)
+            use reader = new StreamReader(ctx.Request.Body, Encoding.UTF8, leaveOpen = true)
+            return! reader.ReadToEndAsync()
+        }
+
+    /// <summary>
+    /// Reads the entire body of the <see cref="Microsoft.AspNetCore.Http.HttpRequest"/> asynchronously and returns it as a <see cref="System.String"/> value. This function let's you decide if you want to leave the ctx.Request.Body element open or not.
+    /// </summary>
+    /// <param name="ctx">The current http context object.</param>
+    /// <param name="leaveOpen">`true` to leave the stream open after the StreamReader object is disposed; otherwise, `false`.</param>
+    /// <returns>Returns the contents of the request body as a <see cref="System.Threading.Tasks.Task{System.String}"/>.</returns>
+    [<Extension>]
+    static member ReadBodyFromRequestAsync(ctx: HttpContext, leaveOpen: bool) =
+        task {
+            use reader =
+                new StreamReader(ctx.Request.Body, Encoding.UTF8, leaveOpen = leaveOpen)
+
             return! reader.ReadToEndAsync()
         }
 

tests/Giraffe.Tests/Helpers.fs

@@ -196,9 +196,14 @@ let hasLastModified (lastModified: DateTimeOffset) (response: HttpResponseMessag
     Assert.Equal(lastModified, response.Content.Headers.LastModified.Value)
     response
 
+let getReqBody (ctx: HttpContext) =
+    ctx.Request.Body.Position <- 0L
+    use reader = new StreamReader(ctx.Request.Body, Encoding.UTF8, leaveOpen = true)
+    reader.ReadToEnd()
+
 let getBody (ctx: HttpContext) =
     ctx.Response.Body.Position <- 0L
-    use reader = new StreamReader(ctx.Response.Body, Encoding.UTF8)
+    use reader = new StreamReader(ctx.Response.Body, Encoding.UTF8, leaveOpen = true)
     reader.ReadToEnd()
 
 let readText (response: HttpResponseMessage) = response.Content.ReadAsStringAsync()

tests/Giraffe.Tests/HttpContextExtensionsTests.fs

@@ -289,3 +289,112 @@ let ``WriteHtmlViewAsync should add html to the context`` () =
         | None -> assertFailf "Result was expected to be %s" expected
         | Some ctx -> Assert.Equal(expected, getBody ctx)
     }
+
+[<Fact>]
+let ``ReadBodyFromRequestAsync is not incorrectly disposing the request body`` () =
+    let ctx = Substitute.For<HttpContext>()
+
+    let bodyStr = "Hello world!"
+    let mutable reqBodyDisposed = false
+
+    let testHandler: HttpHandler =
+        fun (_: HttpFunc) (ctx: HttpContext) ->
+            task {
+                let! bodyFstRead = ctx.ReadBodyFromRequestAsync()
+                Assert.Equal(bodyStr, bodyFstRead)
+                Assert.False reqBodyDisposed
+
+                // By the second time, the request body was read, so the position will be at the end of
+                // the stream. Due to it, we need to get back to the beginning of the stream:
+                ctx.Request.Body.Position <- 0L
+                let! bodySndRead = ctx.ReadBodyFromRequestAsync()
+                Assert.Equal(bodyStr, bodySndRead)
+                Assert.False reqBodyDisposed
+
+                // If we don't change the position, it will simply return an empty string.
+                let! bodyThirdRead = ctx.ReadBodyFromRequestAsync()
+                Assert.Equal("", bodyThirdRead)
+                Assert.False reqBodyDisposed
+
+                return! Encoding.UTF8.GetBytes bodyFstRead |> ctx.WriteBytesAsync
+            }
+
+    let app = route "/" >=> testHandler
+
+    ctx.Request.Method.ReturnsForAnyArgs "GET" |> ignore
+    ctx.Request.Path.ReturnsForAnyArgs(PathString("/")) |> ignore
+
+    ctx.Request.Body <-
+        { new MemoryStream(buffer = Encoding.UTF8.GetBytes(bodyStr)) with
+            member this.Dispose(disposing: bool) : unit =
+                reqBodyDisposed <- true
+                base.Dispose(disposing: bool)
+        }
+
+    ctx.Response.Body <- new MemoryStream()
+
+    task {
+        let! result = app (Some >> Task.FromResult) ctx
+
+        match result with
+        | None -> assertFail "Result was expected to be Option.Some"
+        | Some ctx ->
+            let ctxReqBody = getReqBody ctx
+            Assert.Equal(bodyStr, ctxReqBody)
+            Assert.False reqBodyDisposed // not disposed yet
+
+        do ctx.Request.Body.Dispose() // "manually" disposed
+        Assert.True reqBodyDisposed // now it's disposed
+    }
+
+[<Fact>]
+let ``Old ReadBodyFromRequestAsync implementation was incorrectly disposing the request body`` () =
+    let ctx = Substitute.For<HttpContext>()
+
+    let bodyStr = "Hello world!"
+    let mutable reqBodyDisposed = false
+    let leaveOpen = false // the old default
+
+    let testHandler: HttpHandler =
+        fun (_: HttpFunc) (ctx: HttpContext) ->
+            task {
+                Assert.False reqBodyDisposed
+
+                let! bodyFstRead = ctx.ReadBodyFromRequestAsync(leaveOpen)
+                Assert.Equal(bodyStr, bodyFstRead)
+                Assert.True reqBodyDisposed
+
+                let! capturedException =
+                    Assert.ThrowsAsync<ArgumentException>(fun () -> ctx.ReadBodyFromRequestAsync(leaveOpen))
+
+                Assert.Equal("Stream was not readable.", capturedException.Message)
+
+                return! Encoding.UTF8.GetBytes bodyFstRead |> ctx.WriteBytesAsync
+            }
+
+    let app = route "/" >=> testHandler
+
+    ctx.Request.Method.ReturnsForAnyArgs "GET" |> ignore
+    ctx.Request.Path.ReturnsForAnyArgs(PathString("/")) |> ignore
+
+    ctx.Request.Body <-
+        { new MemoryStream(buffer = Encoding.UTF8.GetBytes(bodyStr)) with
+            member this.Dispose(disposing: bool) : unit =
+                reqBodyDisposed <- true
+                base.Dispose(disposing: bool)
+        }
+
+    ctx.Response.Body <- new MemoryStream()
+
+    task {
+        let! result = app (Some >> Task.FromResult) ctx
+
+        match result with
+        | None -> assertFail "Result was expected to be Option.Some"
+        | Some ctx ->
+            let capturedException =
+                Assert.Throws<ObjectDisposedException>(fun () -> getReqBody ctx |> ignore)
+
+            Assert.Equal("Cannot access a closed Stream.", capturedException.Message)
+            Assert.True reqBodyDisposed
+    }