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

Fix ReadBodyFromRequestAsync disposing ctx.Request.Body #615

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

64J0
Copy link
Member

@64J0 64J0 commented Sep 6, 2024

Description

As noticed by @xperiandri, the ReadBodyFromRequestAsync seems to be incorrectly disposing the ctx.Request.Body. I'm adopting the suggestion from @Banashek, to use the leaveOpen = true parameter.

From the docs:

Unless you set the leaveOpen parameter to true, the StreamReader object calls Dispose() on the provided Stream object when StreamReader.Dispose is called.

How to test

Check the automated tests.

*Thanks @Banashek and @1eyewonder for the comments and ideas.

Related issues

@64J0 64J0 requested review from dbrattli, nojaf and mrtz-j September 6, 2024 22:23
@64J0 64J0 self-assigned this Sep 6, 2024
@64J0
Copy link
Member Author

64J0 commented Sep 6, 2024

I'm not sure how to test this properly. Ideas are welcome!

@Banashek
Copy link
Contributor

Banashek commented Sep 7, 2024

The original bug report was that the body gets disposed.

I would think that asserting the presence of the body post-use would be a good test.

@1eyewonder
Copy link
Contributor

@64J0 Here is a similar type of scenario I had to add over in FsToolkit.ErrorHandling to deal with dispose issues demystifyfp/FsToolkit.ErrorHandling#271 if you were still looking for some unit test ideas

@64J0
Copy link
Member Author

64J0 commented Sep 15, 2024

Thanks for the ideas folks! I'll work on it during this week

…en using ReadBodyFromRequestAsync, and add a new ReadBodyFromRequestAsync function that let's the user define the value of the leaveOpen parameter (useful for tests)
@64J0 64J0 force-pushed the fix-stream-reader-dispose branch from 21ded48 to 18152ca Compare September 20, 2024 01:04
@64J0 64J0 marked this pull request as ready for review September 20, 2024 01:05
tests/Giraffe.Tests/Helpers.fs Show resolved Hide resolved
@64J0 64J0 merged commit d07022b into master Sep 20, 2024
10 checks passed
@64J0 64J0 deleted the fix-stream-reader-dispose branch September 20, 2024 20:09
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.

Never decalre reader with use on ctx.Request.Body
4 participants