Replace dependency on RequestStream in Request.Body with Stream

[danbarua]

Prerequisites

• I have written a descriptive pull-request title
• I have verified that there are no overlapping pull-requests open - overlaps with Non buffered requests #1920
• I have verified that I am following the Nancy code style guidelines
• I have provided test coverage for my change (where applicable)

Description

Use case:
We're uploading large blob files via a Nancy API
The default RequestStream implementation spools out to disk where the size of the incoming request is >85kb to avoid pressure on the Large Object Heap
When requests are aborted, or generally when errors occur, these temp files can leak and cause %TEMPDIR% to run out of space. (I have submitted a few PRs around this in the past but it seems we can't avoid this behaviour, we can't clean up the file in the RequestStream destructor as in the finalizer we have lost the reference to the File)
Since RequestStream is a drop-in-replacement for Stream, it seems logical to use the base class as the Body property on Request, this will allow me to swap in a different Stream implementation such as https://github.com/Microsoft/Microsoft.IO.RecyclableMemoryStream
This also includes tracing so I can troubleshoot where Nancy may be leaking the Request.Body stream.
I can't inherit from/override RequestStream since it has no paramaterless ctor, and this would be a cleaner approach anyhow.

Warning: possible conflicts with #1920

[danbarua]

With this change, I can swap out RequestStream with another implementation here:

Nancy/src/Nancy/Owin/NancyMiddleware.cs

Line 84 in a9989a6

var nancyRequestStream = new RequestStream(owinRequestBody, ExpectedLength(owinRequestHeaders), StaticConfiguration.DisableRequestStreamSwitching ?? false);

[danbarua]

I've reviewed #1920 and I'm fairly sure this change will have no impact on that PR.

[jchannon]

This looks fine to me, we will still use RequestStream but others can use another inherited Stream class and write their own Middleware extension to use that

[danbarua]

Assuming this is ok to merge, any chance of getting it backported to 1.x?

[jchannon]

Pinging @thecodejunkie

On Wednesday, 3 August 2016, Dan Barua notifications@github.com wrote:

Assuming this is ok to merge, any chance of getting it backported to 1.x?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2512 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGaplTjrr5DRtbQb1Z8tg83d3FB4slHks5qcMFsgaJpZM4JP3Fm
.

[horsdal]

Agree that this is good to go. Merging