Fixed StreamResponse bug with seek position #589

Merged
merged 1 commit into from Apr 11, 2012

Projects

None yet

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 Apr 11, 2012
@thecodejunkie
Custodians of the Super-Duper-Happy-Path member

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
Custodians of the Super-Duper-Happy-Path member

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
Custodians of the Super-Duper-Happy-Path member
@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
Custodians of the Super-Duper-Happy-Path member
@thecodejunkie
Custodians of the Super-Duper-Happy-Path member

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