From ee82dcf1a75f1aa06601f89a322640c7b4c8036b Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Tue, 20 Nov 2018 12:22:33 -0800 Subject: [PATCH 1/3] Add `Length` override in `SeekableBufferredRequestStream` - #197 --- .../SeekableBufferedRequestStream.cs | 25 ++++++- .../SeekableBufferedRequestStreamTest.cs | 65 +++++++++++++++++-- 2 files changed, 82 insertions(+), 8 deletions(-) diff --git a/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs b/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs index 59ef5005f..2025df144 100644 --- a/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs +++ b/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs @@ -36,6 +36,26 @@ public override bool CanSeek } } + public override long Length + { + get + { + ThrowIfDisposed(); + if (!_isReadToEndComplete && base.Length == 0) + { + // Because CanSeek is true, HttpContent subclasses assume this property can be used. But, + // InnerStream is initially non-seekable and InnerStream.Length may be incorrect (0). Force + // draining the original Stream (swapping the streams) to make HttpContent assumptions valid. + Seek(1L, SeekOrigin.Current); + + // Leave the stream where it started. + Seek(-1L, SeekOrigin.Current); + } + + return base.Length; + } + } + public override long Position { get @@ -117,8 +137,9 @@ public override long Seek(long offset, SeekOrigin origin) newPosition = currentPosition + offset; break; case SeekOrigin.End: - // We have to check Length here because we might not know the length in some scenarios. - // If we don't know, then we just do the safe thing and force a read to end. + // We have to check Length here because we might not know the length in some scenarios. + // If we don't know, then we just do the safe thing and force a read to end. Length may + // call Seek but only when we haven't already read to end and not with this SeekOrigin. if (Length >= 0) { newPosition = Length + offset; diff --git a/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs b/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs index 216f5f200..bcf4f5132 100644 --- a/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs +++ b/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs @@ -1,11 +1,8 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; -using System.Collections.Generic; using System.ComponentModel; using System.IO; -using System.Linq; using System.Text; using System.Threading.Tasks; using Microsoft.TestCommon; @@ -22,6 +19,40 @@ public class SeekableBufferedRequestStreamTest /// private const int BufferSize = 3; + [Fact] + public void Length_DoesNotSwapStreams_IfNonSeekableLengthIsNotZero() + { + // Arrange + var nonSeekable = CreateNonSeekableStream(Content); + var seekable = CreateSeekableStream(Content); + var stream = CreateStream(nonSeekable, seekable); + + // Act + var length = stream.Length; + + // Assert + Assert.Same(nonSeekable, stream.InnerStream); + Assert.Equal(Content.Length, length); + Assert.Equal(0L, stream.Position); + } + + [Fact] + public void Length_SwapsStreams_IfNonSeekableLengthZero() + { + // Arrange + var nonSeekable = CreateNonSeekableStreamWithZeroLength(Content); + var seekable = CreateSeekableStream(Content); + var stream = CreateStream(nonSeekable, seekable); + + // Act + var length = stream.Length; + + // Assert + Assert.Same(seekable, stream.InnerStream); + Assert.Equal(Content.Length, length); + Assert.Equal(0L, stream.Position); + } + [Fact] public void ReadToEnd_WithRead_SwapsStreams() { @@ -227,7 +258,7 @@ public void Seek_ThrowsOnInvalidSeekOrigin() var origin = (SeekOrigin)5; - var message = + var message = "The value of argument 'origin' (" + (int)origin + ") is invalid for Enum type " + "'SeekOrigin'." + Environment.NewLine + "Parameter name: origin"; @@ -246,6 +277,11 @@ private Stream CreateNonSeekableStream(string content) return new NonSeekableStream(Encoding.UTF8.GetBytes(content)); } + private Stream CreateNonSeekableStreamWithZeroLength(string content) + { + return new NonSeekableStream(Encoding.UTF8.GetBytes(content), hasZeroLength: true); + } + private AccessibleStreamWrapper CreateStream(Stream stream1, Stream stream2) { // Guards @@ -277,13 +313,17 @@ public AccessibleStreamWrapper(HttpRequestBase request) private class NonSeekableStream : MemoryStream { - public NonSeekableStream() + private readonly bool _hasZeroLength; + + public NonSeekableStream(byte[] bytes) + : this(bytes, hasZeroLength: false) { } - public NonSeekableStream(byte[] bytes) + public NonSeekableStream(byte[] bytes, bool hasZeroLength) : base(bytes) { + _hasZeroLength = hasZeroLength; } public override bool CanSeek @@ -293,6 +333,19 @@ public override bool CanSeek return false; } } + + public override long Length + { + get + { + if (_hasZeroLength) + { + return 0L; + } + + return base.Length; + } + } } } } From 258c52230aff6feecda9d60e7340b19fc160b741 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Wed, 21 Nov 2018 09:32:48 -0800 Subject: [PATCH 2/3] Revert "Add `Length` override in `SeekableBufferredRequestStream`" This reverts commit ee82dcf1a75f1aa06601f89a322640c7b4c8036b. --- .../SeekableBufferedRequestStream.cs | 25 +------ .../SeekableBufferedRequestStreamTest.cs | 65 ++----------------- 2 files changed, 8 insertions(+), 82 deletions(-) diff --git a/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs b/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs index 2025df144..59ef5005f 100644 --- a/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs +++ b/src/System.Web.Http.WebHost/SeekableBufferedRequestStream.cs @@ -36,26 +36,6 @@ public override bool CanSeek } } - public override long Length - { - get - { - ThrowIfDisposed(); - if (!_isReadToEndComplete && base.Length == 0) - { - // Because CanSeek is true, HttpContent subclasses assume this property can be used. But, - // InnerStream is initially non-seekable and InnerStream.Length may be incorrect (0). Force - // draining the original Stream (swapping the streams) to make HttpContent assumptions valid. - Seek(1L, SeekOrigin.Current); - - // Leave the stream where it started. - Seek(-1L, SeekOrigin.Current); - } - - return base.Length; - } - } - public override long Position { get @@ -137,9 +117,8 @@ public override long Seek(long offset, SeekOrigin origin) newPosition = currentPosition + offset; break; case SeekOrigin.End: - // We have to check Length here because we might not know the length in some scenarios. - // If we don't know, then we just do the safe thing and force a read to end. Length may - // call Seek but only when we haven't already read to end and not with this SeekOrigin. + // We have to check Length here because we might not know the length in some scenarios. + // If we don't know, then we just do the safe thing and force a read to end. if (Length >= 0) { newPosition = Length + offset; diff --git a/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs b/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs index bcf4f5132..216f5f200 100644 --- a/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs +++ b/test/System.Web.Http.WebHost.Test/SeekableBufferedRequestStreamTest.cs @@ -1,8 +1,11 @@ // Copyright (c) .NET Foundation. All rights reserved. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; +using System.Collections.Generic; using System.ComponentModel; using System.IO; +using System.Linq; using System.Text; using System.Threading.Tasks; using Microsoft.TestCommon; @@ -19,40 +22,6 @@ public class SeekableBufferedRequestStreamTest /// private const int BufferSize = 3; - [Fact] - public void Length_DoesNotSwapStreams_IfNonSeekableLengthIsNotZero() - { - // Arrange - var nonSeekable = CreateNonSeekableStream(Content); - var seekable = CreateSeekableStream(Content); - var stream = CreateStream(nonSeekable, seekable); - - // Act - var length = stream.Length; - - // Assert - Assert.Same(nonSeekable, stream.InnerStream); - Assert.Equal(Content.Length, length); - Assert.Equal(0L, stream.Position); - } - - [Fact] - public void Length_SwapsStreams_IfNonSeekableLengthZero() - { - // Arrange - var nonSeekable = CreateNonSeekableStreamWithZeroLength(Content); - var seekable = CreateSeekableStream(Content); - var stream = CreateStream(nonSeekable, seekable); - - // Act - var length = stream.Length; - - // Assert - Assert.Same(seekable, stream.InnerStream); - Assert.Equal(Content.Length, length); - Assert.Equal(0L, stream.Position); - } - [Fact] public void ReadToEnd_WithRead_SwapsStreams() { @@ -258,7 +227,7 @@ public void Seek_ThrowsOnInvalidSeekOrigin() var origin = (SeekOrigin)5; - var message = + var message = "The value of argument 'origin' (" + (int)origin + ") is invalid for Enum type " + "'SeekOrigin'." + Environment.NewLine + "Parameter name: origin"; @@ -277,11 +246,6 @@ private Stream CreateNonSeekableStream(string content) return new NonSeekableStream(Encoding.UTF8.GetBytes(content)); } - private Stream CreateNonSeekableStreamWithZeroLength(string content) - { - return new NonSeekableStream(Encoding.UTF8.GetBytes(content), hasZeroLength: true); - } - private AccessibleStreamWrapper CreateStream(Stream stream1, Stream stream2) { // Guards @@ -313,17 +277,13 @@ public AccessibleStreamWrapper(HttpRequestBase request) private class NonSeekableStream : MemoryStream { - private readonly bool _hasZeroLength; - - public NonSeekableStream(byte[] bytes) - : this(bytes, hasZeroLength: false) + public NonSeekableStream() { } - public NonSeekableStream(byte[] bytes, bool hasZeroLength) + public NonSeekableStream(byte[] bytes) : base(bytes) { - _hasZeroLength = hasZeroLength; } public override bool CanSeek @@ -333,19 +293,6 @@ public override bool CanSeek return false; } } - - public override long Length - { - get - { - if (_hasZeroLength) - { - return 0L; - } - - return base.Length; - } - } } } } From 03ed1ede4ad89abf0c3c38cc215b2fb2a9b9f151 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Wed, 21 Nov 2018 10:22:02 -0800 Subject: [PATCH 3/3] Override `TryComputeLength(...)` in web host `HttpContent` implementations - other implementations in this repo are already fine --- .../HttpControllerHandler.cs | 16 +++++++--- .../HttpControllerHandlerTest.cs | 31 +++++++++++++------ 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/System.Web.Http.WebHost/HttpControllerHandler.cs b/src/System.Web.Http.WebHost/HttpControllerHandler.cs index 9a78f57f3..213f0e982 100644 --- a/src/System.Web.Http.WebHost/HttpControllerHandler.cs +++ b/src/System.Web.Http.WebHost/HttpControllerHandler.cs @@ -717,14 +717,17 @@ public Task WriteToStreamAsync(Stream stream, TransportContext context) return SerializeToStreamAsync(stream, context); } - public bool TryCalculateLength(out long length) + public Task GetContentReadStreamAsync() { - return TryComputeLength(out length); + return CreateContentReadStreamAsync(); } - public Task GetContentReadStreamAsync() + protected override bool TryComputeLength(out long length) { - return CreateContentReadStreamAsync(); + // Do not attempt to calculate length because SeekableBufferedRequestStream (for example) + // may report 0 until the underlying Stream has been read to end. + length = 0L; + return false; } } @@ -762,7 +765,10 @@ protected override Task CreateContentReadStreamAsync() protected override bool TryComputeLength(out long length) { - return StreamContent.TryCalculateLength(out length); + // Do not attempt to calculate length because SeekableBufferedRequestStream (for example) + // may report 0 until the underlying Stream has been read to end. + length = 0L; + return false; } } } diff --git a/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs b/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs index c360f8e5a..7a991490c 100644 --- a/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs +++ b/test/System.Web.Http.WebHost.Test/HttpControllerHandlerTest.cs @@ -73,6 +73,24 @@ public void ConvertRequest_Creates_HttpRequestMessage_For_All_HttpMethods(HttpMe Assert.Equal(httpMethod, request.Method); } + [Fact] + public void ConvertRequest_DoesNotAddContentLength() + { + // Arrange + HttpContextBase contextBase = CreateStubContextBase("Get", new MemoryStream()); + + // Act + HttpRequestMessage request = HttpControllerHandler.ConvertRequest(contextBase); + + // Assert + var headers = request.Content.Headers; + Assert.NotNull(headers); + Assert.Null(headers.ContentLength); + + IEnumerable unused; + Assert.False(headers.TryGetValues("Content-Length", out unused)); + } + [Fact] public void ConvertRequest_Copies_Headers_And_Content_Headers() { @@ -130,8 +148,10 @@ public void ConvertRequest_AddsOwinEnvironment_WhenPresentInHttpContext() { HttpRequestBase stubRequest = CreateStubRequestBase("IgnoreMethod", ignoreStream); IDictionary expectedEnvironment = new Dictionary(); - IDictionary items = new Hashtable(); - items.Add(HttpControllerHandler.OwinEnvironmentHttpContextKey, expectedEnvironment); + IDictionary items = new Hashtable + { + { HttpControllerHandler.OwinEnvironmentHttpContextKey, expectedEnvironment } + }; HttpContextBase context = CreateStubContextBase(stubRequest, items); // Act @@ -1946,13 +1966,6 @@ private static HttpResponseBase CreateStubResponseBase() return new Mock().Object; } - private static HttpResponseBase CreateStubResponseBase(CancellationToken clientDisconnectedToken) - { - Mock mock = new Mock(); - mock.Setup(r => r.ClientDisconnectedToken).Returns(clientDisconnectedToken); - return mock.Object; - } - private static HttpResponseBase CreateStubResponseBase(Stream outputStream) { Mock responseBaseMock = new Mock();