Extract awaitable support into a struct #1207

pakrym · 2017-02-09T23:44:29Z

halter73 · 2017-02-09T23:51:57Z

davidfowl

Looks good, small changes in the review comments.

We should see if we can get cancellation in there.

davidfowl · 2017-02-10T00:46:42Z

src/System.IO.Pipelines/Pipe.cs

+                    _awaitableIsCompleted);
+            }
+
+            public bool IsCompleted() => ReferenceEquals(_state, _awaitableIsCompleted);


davidfowl · 2017-02-10T01:21:51Z

src/System.IO.Pipelines/Pipe.cs

-            _readerCallback = _awaitableIsNotCompleted;
-            _writerCallback = _awaitableIsCompleted;
+
+            _readerAwaitable = new Awaitable(false, options.ReaderScheduler ?? InlineScheduler.Default);


Use optional parameter syntax and put the bool last. Makes it more obvious what it is.

halter73 · 2017-02-10T20:51:59Z

src/System.IO.Pipelines/Pipe.Awaitable.cs

+{
+    internal partial class Pipe
+    {
+        private struct Awaitable


Nit: Make PipeAwaitable a non-nested internal struct instead of using partial classes.

halter73 · 2017-02-10T20:54:15Z

src/System.IO.Pipelines/Pipe.OperationState.cs

+                public static int NotActive = 0;
+                public static int Active = 1;
+            }
+


davidfowl · 2017-02-10T22:13:59Z

src/System.IO.Pipelines/Pipe.cs

@@ -448,8 +395,6 @@ void IPipeReader.Advance(ReadCursor consumed, ReadCursor examined)
                _length -= consumedBytes;
                resumeWriter = _length < _maximumSizeLow;

-                // Change the state from observed -> not cancelled. We only want to reset the cancelled state if it was observed
-                Interlocked.CompareExchange(ref _cancelledState, CancelledState.NotCancelled, CancelledState.CancellationObserved);


Where does this happen?

PipeAwaitable.Reset

davidfowl · 2017-02-10T22:14:58Z

src/System.IO.Pipelines/System.IO.Pipelines.csproj

@@ -5,6 +5,7 @@
    <TargetFrameworks>net451;netstandard1.3</TargetFrameworks>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <PackageIconUrl>http://go.microsoft.com/fwlink/?linkid=833199</PackageIconUrl>
+    <DefineConstants Condition="'$(Configuration)' == 'Debug'">OPERATION_LOCATION_TRACKING;COMPLETION_LOCATION_TRACKING</DefineConstants>


You need to keep the default <DefineConstants>.

<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);OPERATION_LOCATION_TRACKING;COMPLETION_LOCATION_TRACKING</DefineConstants>

benaadams · 2017-02-10T22:54:20Z

src/System.IO.Pipelines/PipeAwaitable.cs

+                CancelledState.NotCancelled,
+                CancelledState.CancellationObserved);
+
+            if (_cancelledState != CancelledState.CancellationRequested)


Take return value from the CompareExchange above and use that instead? Rather than re-reading the value that's just been cpu pipeline invalidated

It's done in other PR: #1209

Concurrent PRs on concurrency, nice touch 😉

I don't think this PR is meant to affect any of the logic. It's about code cleanliness.

davidfowl · 2017-02-10T23:13:12Z

src/System.IO.Pipelines/Pipe.cs

@@ -10,20 +10,20 @@ namespace System.IO.Pipelines
    /// <summary>
    /// Default <see cref="IPipeWriter"/> and <see cref="IPipeReader"/> implementation.
    /// </summary>
-    internal class Pipe : IPipe, IPipeReader, IPipeWriter, IReadableBufferAwaiter, IWritableBufferAwaiter
+    internal partial class Pipe : IPipe, IPipeReader, IPipeWriter, IReadableBufferAwaiter, IWritableBufferAwaiter


Remove the partial?

davidfowl

LGTM

Extract awaitable support into a struct

89242f9

dnfclas added the cla-already-signed label Feb 9, 2017

davidfowl reviewed Feb 10, 2017

View reviewed changes

pakrym added 5 commits February 10, 2017 09:24

Property and parameter order

031fd52

Cancelation state to Awaitable

736d3d8

Slit files

6474c11

Locations

5fdc44a

Extract operation state and pass exceptionresource

c833fe2

halter73 reviewed Feb 10, 2017

View reviewed changes

src/System.IO.Pipelines/Pipe.OperationState.cs Outdated

public static int NotActive = 0;

public static int Active = 1;

}

Copy link

Member

halter73 Feb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

davidfowl reviewed Feb 10, 2017

View reviewed changes

Uninternalize Pipe primitives.

6d25cde

benaadams reviewed Feb 10, 2017

View reviewed changes

Keep global DefineConstants

f7e401c

pakrym force-pushed the pakrym/extract-awaitable branch from 2bf77b5 to f7e401c Compare February 10, 2017 23:07

davidfowl reviewed Feb 10, 2017

View reviewed changes

pakrym added 2 commits February 10, 2017 15:24

ToStrings and cleanup

634d834

Disable location tracking by default, it slows down tests to much.

62e2174

davidfowl approved these changes Feb 10, 2017

View reviewed changes

pakrym merged commit b150a99 into master Feb 11, 2017

pakrym deleted the pakrym/extract-awaitable branch February 28, 2017 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract awaitable support into a struct #1207

Extract awaitable support into a struct #1207

pakrym commented Feb 9, 2017

halter73 commented Feb 9, 2017

davidfowl left a comment

davidfowl Feb 10, 2017

davidfowl Feb 10, 2017

halter73 Feb 10, 2017

halter73 Feb 10, 2017

davidfowl Feb 10, 2017

pakrym Feb 10, 2017

davidfowl Feb 10, 2017 •

edited

Loading

benaadams Feb 10, 2017 •

edited

Loading

pakrym Feb 10, 2017

benaadams Feb 10, 2017

halter73 Feb 10, 2017

davidfowl Feb 10, 2017

davidfowl left a comment

Extract awaitable support into a struct #1207

Extract awaitable support into a struct #1207

Conversation

pakrym commented Feb 9, 2017

halter73 commented Feb 9, 2017

davidfowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

benaadams Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

davidfowl Feb 10, 2017 •

edited

Loading

benaadams Feb 10, 2017 •

edited

Loading