Add boolean dispose flag in HttpRequestStreamReader/ResponseWriter (#540) #898

jkotalik · 2017-07-13T18:20:21Z

Remove nulling out fields in favor of a dispose boolean.
Fixes #540

moozzyk · 2017-07-13T18:24:52Z

src/Microsoft.AspNetCore.WebUtilities/HttpRequestStreamReader.cs

(shameless plug) - install this: https://visualstudiogallery.msdn.microsoft.com/bcb3ef8e-a3a4-401e-9e63-fdf00b45268f and you won't make this mistake again.

Tratcher · 2017-07-13T18:25:27Z

Titles need a description.

Tratcher · 2017-07-14T21:21:33Z

let's do the response stream as well, and check if any other near by components have the same problem.

jkotalik · 2017-07-15T00:19:57Z

🆙 📅 based on what @Tratcher mentioned. Also added tests and modified both files to be consistent.

Tratcher · 2017-07-17T16:27:02Z

src/Microsoft.AspNetCore.WebUtilities/HttpRequestStreamReader.cs

                if (_charBuffer != null)
                {
                    _charPool.Return(_charBuffer);
                    _charBuffer = null;


remove these nulls as well.

Tratcher · 2017-07-17T16:31:44Z

src/Microsoft.AspNetCore.WebUtilities/HttpRequestStreamReader.cs

-                _stream = null;
-
+                _disposed = true;
                if (_bytePool != null)


why are there null checks on _bytePool and _charPool here? The constructor prevents nulls.

I think they were from before when we deliberately null out these streams.

Except _bytePool wasn't being nulled out before.

Tratcher · 2017-07-17T16:37:02Z

test/Microsoft.AspNetCore.WebUtilities.Tests/HttpRequestStreamReaderTest.cs

+        }
+
+        [Fact]
+        public static void NegativeBufferSize_ExpectArgumentOutOfRangeException()


dnfclas · 2017-07-17T17:24:52Z

@jkotalik, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

Eilon · 2017-07-17T18:46:58Z

src/Microsoft.AspNetCore.WebUtilities/HttpRequestStreamReader.cs

+            if (_disposed)
            {
-                throw new ObjectDisposedException("stream");
+                throw new ObjectDisposedException(nameof(HttpRequestStreamReader));


I wouldn't pass anything into the ctor here. The type name is not the "object name" - it's pretty useless here and generates noise.

ObjectDisposedException doesn't actually have a ctor with no arguments. We seem to do the nameof(classname) across our repos.

Eh, that's fine. I looked through the CoreFX repo and in there it's a mix of null, GetType().Name, and other stuff like that. So nameof(this_class_name) seems fine.

Eilon · 2017-07-17T18:47:41Z

test/Microsoft.AspNetCore.WebUtilities.Tests/HttpRequestStreamReaderTest.cs

            Assert.Null(eol);
        }

+        [Theory, MemberData(nameof(HttpRequestStreamReaderData))]


Consider putting the attributes on separate lines.

Eilon · 2017-07-19T17:19:52Z

Would it make sense to add tests that check for ObjectDisposedException? If that's a "feature" of this type, should we not test it?

jkotalik · 2017-07-19T17:21:07Z

Sounds reasonable, I'll add some in.

jkotalik · 2017-07-27T02:03:28Z

@Tratcher @Eilon 🆙 📅 . I don't believe the async version can be tested in Assert.Throws, but please correct me if I'm wrong.

Tratcher · 2017-07-27T16:11:37Z

There's an Assert.ThrowsAsync

Tratcher · 2017-07-27T17:25:46Z

test/Microsoft.AspNetCore.WebUtilities.Tests/HttpRequestStreamReaderTest.cs

+            var httpRequestStreamReader = new HttpRequestStreamReader(new MemoryStream(), Encoding.UTF8, 10, ArrayPool<byte>.Shared, ArrayPool<char>.Shared);
+            httpRequestStreamReader.Dispose();
+
+            Assert.ThrowsAsync<ObjectDisposedException>(async () =>


await this (and make the method public async Task)

The functions below are already async. I don't understand the value in doing this.

yield return new object[] { new Func<HttpResponseStreamWriter, Task>(async (httpResponseStreamWriter) => { await httpResponseStreamWriter.WriteAsync('a'); })};

Tratcher · 2017-07-27T19:23:45Z

test/Microsoft.AspNetCore.WebUtilities.Tests/HttpRequestStreamReaderTest.cs


+        [Theory]
+        [MemberData(nameof(HttpRequestNullData))]
+        public static void NullInputsInConstructor_ExpectArgumentNullException(Stream stream, Encoding encoding, ArrayPool<byte> bytePool, ArrayPool<char> charPool)


Why are your test methods static?

All of them were static to begin with, should I change it in another PR?

you can leave it for now, it's just odd.

static means the test class's ctor won't run. Of course, if there isn't one, that's no problem. As long as the test runs and it passes, it's probably good 😄

Tratcher · 2017-07-27T19:24:37Z

test/Microsoft.AspNetCore.WebUtilities.Tests/HttpRequestStreamReaderTest.cs

+        }
+
+        [Fact]
+        public static async void StreamDisposed_ExpectObjectDisposedExceptionAsync()


async Task, never async void.

jkotalik · 2017-07-27T21:25:02Z

@Eilon this good to go? 😄

jkotalik requested review from Tratcher and ryanbrandenburg July 13, 2017 18:20

dnfclas added the cla-not-required label Jul 13, 2017

moozzyk reviewed Jul 13, 2017

View reviewed changes

jkotalik changed the title ~~Fixes #540~~ Add boolean dispose flag in HttpRequestStreamReader #540 Jul 13, 2017

jkotalik changed the title ~~Add boolean dispose flag in HttpRequestStreamReader #540~~ Add boolean dispose flag in HttpRequestStreamReader [#540] Jul 13, 2017

jkotalik removed the request for review from ryanbrandenburg July 13, 2017 22:25

jkotalik changed the title ~~Add boolean dispose flag in HttpRequestStreamReader [#540]~~ Add boolean dispose flag in HttpRequestStreamReader (#540) Jul 14, 2017

jkotalik mentioned this pull request Jul 14, 2017

HttpResponseStreamWriter nulls out fields in Dispose #900

Closed

jkotalik changed the title ~~Add boolean dispose flag in HttpRequestStreamReader (#540)~~ Add boolean dispose flag in HttpRequestStreamReader/ResponseWriter (#540) Jul 15, 2017

Tratcher assigned jkotalik Jul 17, 2017

Tratcher added this to the 2.1.0 milestone Jul 17, 2017

Tratcher requested a review from rynowak July 17, 2017 16:24

Tratcher suggested changes Jul 17, 2017

View reviewed changes

dnfclas added the cla-signed label Jul 17, 2017

Eilon suggested changes Jul 17, 2017

View reviewed changes

Tratcher approved these changes Jul 17, 2017

View reviewed changes

jkotalik force-pushed the jkotalik/Dispose branch from 59275fa to 7a2c331 Compare July 27, 2017 02:02

Tratcher reviewed Jul 27, 2017

View reviewed changes

Tratcher approved these changes Jul 27, 2017

View reviewed changes

jkotalik force-pushed the jkotalik/Dispose branch 2 times, most recently from d6c9f0c to 239c22d Compare July 27, 2017 19:47

Eilon approved these changes Jul 27, 2017

View reviewed changes

Refactor to HttpRequest/Response Streams

057fc81

jkotalik force-pushed the jkotalik/Dispose branch from 239c22d to 057fc81 Compare July 27, 2017 22:47

jkotalik merged commit 057fc81 into dev Jul 27, 2017

jkotalik deleted the jkotalik/Dispose branch July 27, 2017 22:54

Add boolean dispose flag in HttpRequestStreamReader/ResponseWriter (#540) #898

Add boolean dispose flag in HttpRequestStreamReader/ResponseWriter (#540) #898

Uh oh!

Conversation

jkotalik commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moozzyk Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tratcher commented Jul 13, 2017

Uh oh!

Tratcher commented Jul 14, 2017

Uh oh!

jkotalik commented Jul 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfclas commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eilon commented Jul 19, 2017

Uh oh!

jkotalik commented Jul 19, 2017

Uh oh!

jkotalik commented Jul 27, 2017

Uh oh!

Tratcher commented Jul 27, 2017

Uh oh!

Tratcher Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotalik commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jkotalik commented Jul 13, 2017 •

edited

Loading

moozzyk Jul 13, 2017 •

edited

Loading

Tratcher Jul 27, 2017 •

edited

Loading

jkotalik commented Jul 27, 2017 •

edited

Loading