Non buffered requests

[adamhathcock]

Issue: #1897

RequestStream doesn't buffer by default and is read only. Buffering is done when requested (by Form and Multipart data).

Buffering is only done lazily when Form or Multipart data is accessed on the Request object.

Buffering to a file is still done as before.

Lots of tests relied on the fact that requests buffered and reset positions to 0. Some tests were removed or majorly as they did stuff like test writing to the RequestStream

[cvetomir-todorov]

I am applying this PR to a fork of mine to test it out. I think you should mention that buffering is done when using model binding, too.

I think it may be omitted, if the support for multiple binding is dropped. When I removed the buffering in the default binder there was just a single test failure related to that and nothing else. Or alternatively - a separate method is added Bind(multiple: true) which should trigger the buffering. While the default Bind() (delegating to Bind(multiple: false)) does not.

[adamhathcock]

It looks like I have DefaultBinder buffering always. I guess I'm not too familiar with the binding to make it lazy. Though, I'm not sure why buffering for the model would be bad. You're effectively buffering for the binding anyway.

If you want to make it lazy, I say go for it :)

[cvetomir-todorov]

Well, additional API methods may be out of scope of the current PR, although it is related somehow. Nancy owners should have a major say about this before any implementation is to be added. But I do think that binding once should not require buffering. The question is what the API for multiple binding should look like.

[cvetomir-todorov]

@adamhathcock any particular reason to explicitly not support seeking in request stream? I changed it to the old implementation which asks the underlying this.stream and it works fine for my case but wanted to know the reasoning behind this.

[adamhathcock]

The old implementation always buffers to memory or a file.

I want to only buffer the request body when required because I have scenarios with large request payloads that shouldn't be buffered

[cvetomir-todorov]

I meant these changes in RequestStream class:

    public override bool CanSeek
    {                
        get { return false; }
    }
    public override long Seek(long offset, SeekOrigin origin)
    {
        throw new NotSupportedException();
    }

My question was whether there is a need not to delegate these calls to this.stream as before. If the request stream is not buffered and does not support seeking then it would return false and Seek shouldn't be called by client. When the request stream is buffered though, it may be useful to be able to Seek in the stream.

[adamhathcock]

OH I see what you're saying.

I guess I'm breaking pattern by allowing Position to be set when buffered but not allowing CanSeek and Seek to be used if buffered.

That should change, I'll do that shortly.

[cvetomir-todorov]

I think you had it right and because of the unit tests failing it got reverted back instead of the tests being fixed.

The point is: even non-buffered requests should support seeking if the underlying stream does. That's just read-related and I can't think of a reason why it should be disallowed if the stream supports it. That was the behavior before.

[adamhathcock]

The actual HTTP Request Stream doesn't support seeking. You can only seek when the Stream gets buffered. That's what I'm trying to codify anyway. The fact that the implementation replaces the request stream with another stream object is an implementation detail.

[cvetomir-todorov]

• If the actual HTTP Request Stream does not support streaming its CanSeek property will return false anyway and its Seek method will throw the exception, too. No need to write checks that are not required. And code gets simpler, too.
• If another underlying stream does support seeking though and request is not buffered you're limiting that stream capabilities.
• I think it's not good to be able to seek "only when the Stream gets buffered". You should be able to seek when the stream supports it. That's why there's CanSeek property in the first place.

[adamhathcock]

The RequestStream class isn't supposed to be generic. It's specific to a HTTP Request Stream. It just can't be hardcoded to any specific request stream type.

I like the explicit "IsBuffered" to know that the stream isn't the http request stream anymore.

I get what you're saying but don't think it should support every scenario.

[cvetomir-todorov]

Nancy supports lots of hosts already and adding different hosts is encouraged. So I think it should be as generic as possible and should support every scenario possible. The implementation I suggested is not hard and it's what it is in the main branch. Also, if you want to distinguish the stream - you can always just add an IsBuffered or IsRaw or something-like-that property.

[adamhathcock]

Any new comments or thoughts on this?

[khellang]

Assigning this to 2.0 so we can properly test this as a beta release or something.

[thecodejunkie]

Just to elaborate on what @khellang said. This is touching a very sensitive part of the framework and we need to make sure that it is properly evaluated and tested before it hits a stable build.

[adamhathcock]

No problem. Thanks for looking.

[jchannon]

@adamhathcock can you rebase this. if @NancyFx/most-valued-minions and @thecodejunkie have no issues lets merge this

[adamhathcock]

rebased. Though it looks like the Travis CI has a keyserver timeout issue?

[thecodejunkie]

@jchannon @adamhathcock this #1920 (comment) still applies :D It's going to require some focused testing and not just a code review. It's likely that this won't make it into 2.0 because of time constraints, but rather 2.1 or another point release