Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed StreamResponse bug with seek position #589

Merged
merged 1 commit into from

2 participants

@serbajlo

If passed stream's seek position is at the end, no data were sent over to the client.

@thecodejunkie thecodejunkie merged commit 1ae06a4 into NancyFx:master
@thecodejunkie

I accepted this pull request but then reverted the change because I relalised that we used to have this exact behavior but decided to change it so it

  • Throws an exception if it cannot read (just swallowing it, without notifying the user is bad)
  • Does not re-position the stream (sometimes you may want to return from the current location of the stream and we should not prevent that)

Here is the commit when we first changed the behavior
ba2c77c

@serbajlo
  • Throws an exception if it cannot read (just swallowing it, without notifying the user is bad)

this is a valid point.

  • Does not re-position the stream (sometimes you may want to return from the current location of the stream and we should not prevent that)

I can't recall a single case I'd want to send only a part of any stream. On the other side, having a stream with position at the end of the stream is a very common situation. I believe this should be solved in the StreamResponse directly rather than forcing developers to think about it all the time.

@thecodejunkie

I can't recall the exact specific instance, but we did remove it after having a discussion with a user that needed to stream from the current location. We decided that we really shouldn't manipulate the stream because we do not own it.

The one thing I could see us doing is add an overload with a boolean property, something like rewindStream, and when it's true we reposition it at 0 (if CanSeek is true). The default behavior should be to leave it untouched

@serbajlo

Another possible (easy) solution would be to "rewind" only if the position is at the end of the stream only.

@thecodejunkie
@serbajlo

Consult it with the user sending the partial stream only if he's ok with it. I'll then submit a new pull request that may solve problems of us both.

@thecodejunkie
@thecodejunkie

this is the discussion https://groups.google.com/d/topic/nancy-web-framework/rQ-qSPm8pVU/discussion

I'm debating if it would be better to have it automatically rewind if the stream is at the end, or add the overloaded parameter to control rewinding... I think I'm more prone to the parameter because it does cover the first use case as well + it does give you more fine grained control of the behavior

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 10, 2012
  1. Fixed StreamResponse bug with seek position

    serb authored
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 1 deletion.
  1. +9 −1 src/Nancy/Responses/StreamResponse.cs
View
10 src/Nancy/Responses/StreamResponse.cs
@@ -27,7 +27,15 @@ private static Action<Stream> GetResponseBodyDelegate(Func<Stream> sourceDelegat
{
using (var source = sourceDelegate.Invoke())
{
- source.CopyTo(stream);
+ if (source.CanSeek)
+ {
+ source.Position = 0;
+ }
+
+ if (source.CanRead)
+ {
+ source.CopyTo(stream);
+ }
}
};
}
Something went wrong with that request. Please try again.