Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Replace dependency on RequestStream in Request.Body with Stream #2512

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

danbarua
Copy link
Contributor

Prerequisites

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

Allows swapping out switchover stream behaviour
@danbarua
Copy link
Contributor Author

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

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

@danbarua
Copy link
Contributor Author

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

@jchannon
Copy link
Member

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
Copy link
Contributor Author

danbarua commented Aug 3, 2016

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

@jchannon
Copy link
Member

jchannon commented Aug 3, 2016

Pinging @thecodejunkie

On Wednesday, 3 August 2016, Dan Barua [email protected] 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 horsdal added this to the 2.0-clinteastwood milestone Aug 4, 2016
@horsdal
Copy link
Member

horsdal commented Aug 4, 2016

Agree that this is good to go. Merging

@horsdal horsdal merged commit d76e7ef into NancyFx:master Aug 4, 2016
danbarua added a commit to danbarua/Nancy that referenced this pull request Nov 15, 2016
Allows swapping out switchover stream behaviour

Backport of NancyFx#2512
@xt0rted xt0rted mentioned this pull request Jul 3, 2017
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants