Remove unused cancelled token check, fix PLINQ version of #2239

Author: Clockwork-Muse

src/System.Linq.Parallel/src/System/Linq/Parallel/Utils/ReverseComparer.cs

@@ -27,7 +27,7 @@ internal ReverseComparer(IComparer<T> comparer)
 
         public int Compare(T x, T y)
         {
-            return -_comparer.Compare(x, y);
+            return _comparer.Compare(y, x);
         }
     }
 }

src/System.Linq.Parallel/src/System/Linq/ParallelEnumerable.cs

@@ -352,29 +352,10 @@ public static ParallelQuery<TSource> WithDegreeOfParallelism<TSource>(this Paral
         /// <exception cref="T:System.InvalidOperationException">
         /// WithCancellation is used multiple times in the query.
         /// </exception>
-        /// <exception cref="T:System.ObjectDisposedException">
-        /// The <see cref="T:System.Threading.CancellationTokenSource"/> associated with the <paramref name="cancellationToken"/> has been disposed.
-        /// </exception>
         public static ParallelQuery<TSource> WithCancellation<TSource>(this ParallelQuery<TSource> source, CancellationToken cancellationToken)
         {
             if (source == null) throw new ArgumentNullException("source");
 
-            // also a convenience check whether the cancellationTokenSource backing the token is already disposed.
-            // do this via a dummy registration as there is no public IsDipsosed property on CT.
-            CancellationTokenRegistration dummyRegistration = new CancellationTokenRegistration();
-            try
-            {
-                dummyRegistration = cancellationToken.Register(() => { });
-            }
-            catch (ObjectDisposedException)
-            {
-                throw new ArgumentException(SR.ParallelEnumerable_WithCancellation_TokenSourceDisposed, "cancellationToken");
-            }
-            finally
-            {
-                dummyRegistration.Dispose();
-            }
-
             QuerySettings settings = QuerySettings.Empty;
             settings.CancellationState = new CancellationState(cancellationToken);
 

src/System.Linq.Parallel/tests/Helpers/Comparers.cs

@@ -72,6 +72,24 @@ public int Compare(int x, int y)
         }
     }
 
+    /// <summary>
+    /// Returns an extreme value from non-equal comparisons.
+    /// </summary>
+    /// <remarks>Helper for regression test against PLINQ's version of #2239 .</remarks>
+    /// <typeparam name="T">The type being compared.</typeparam>
+    internal class ExtremeComparer<T> : IComparer<T>
+    {
+        private IComparer<T> _def = Comparer<T>.Default;
+
+        public int Compare(T x, T y)
+        {
+            int direction = _def.Compare(x, y);
+            return direction == 0 ? 0 :
+                direction > 0 ? int.MaxValue :
+                int.MinValue;
+        }
+    }
+
     internal static class DelgatedComparable
     {
         public static DelegatedComparable<T> Delegate<T>(T value, IComparer<T> comparer) where T : IComparable<T>

src/System.Linq.Parallel/tests/QueryOperators/OrderByThenByTests.cs

@@ -326,6 +326,55 @@ public static void OrderByDescending_NotPipelined_CustomComparator_Longrunning(L
             OrderByDescending_NotPipelined_CustomComparer(labeled, count);
         }
 
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        // Regression test for the PLINQ version of #2239 - comparer returning max/min value.
+        public static void OrderBy_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MinValue;
+            foreach (int i in labeled.Item.OrderBy(x => x, new ExtremeComparer<int>()))
+            {
+                Assert.InRange(i, prev, int.MaxValue);
+                prev = i;
+            }
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        // Regression test for the PLINQ version of #2239 - comparer returning max/min value.
+        public static void OrderByDescending_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MaxValue;
+            foreach (int i in labeled.Item.OrderByDescending(x => x, new ExtremeComparer<int>()))
+            {
+                Assert.InRange(i, int.MinValue, prev);
+                prev = i;
+            }
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        public static void OrderBy_NotPipelined_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MinValue;
+            Assert.All(labeled.Item.OrderBy(x => x, new ExtremeComparer<int>()).ToList(), x => { Assert.InRange(x, prev, int.MaxValue); prev = x; });
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        public static void OrderByDescending_NotPipelined_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MaxValue;
+            Assert.All(labeled.Item.OrderByDescending(x => x, new ExtremeComparer<int>()).ToList(), x => { Assert.InRange(x, int.MinValue, prev); prev = x; });
+        }
+
         [Fact]
         public static void OrderBy_ArgumentNullException()
         {
@@ -753,6 +802,55 @@ public static void ThenByDescending_NotPipelined_CustomComparator_Longrunning(La
             ThenByDescending_NotPipelined_CustomComparer(labeled, count);
         }
 
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        // Regression test for the PLINQ version of #2239 - comparer returning max/min value.
+        public static void ThenBy_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MinValue;
+            foreach (int i in labeled.Item.OrderBy(x => 0).ThenBy(x => x, new ExtremeComparer<int>()))
+            {
+                Assert.InRange(i, prev, int.MaxValue);
+                prev = i;
+            }
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        // Regression test for the PLINQ version of #2239 - comparer returning max/min value.
+        public static void ThenByDescending_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MaxValue;
+            foreach (int i in labeled.Item.OrderBy(x => 0).ThenByDescending(x => x, new ExtremeComparer<int>()))
+            {
+                Assert.InRange(i, int.MinValue, prev);
+                prev = i;
+            }
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        public static void ThenBy_NotPipelined_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MinValue;
+            Assert.All(labeled.Item.OrderBy(x => 0).ThenBy(x => x, new ExtremeComparer<int>()).ToList(), x => { Assert.InRange(x, prev, int.MaxValue); prev = x; });
+        }
+
+        [Theory]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(Sources))]
+        [MemberData("OrderByRandomData", (object)(new[] { 0, 1, 2, 16 }))]
+        [MemberData("Ranges", (object)(new int[] { 0, 1, 2, 16 }), MemberType = typeof(UnorderedSources))]
+        public static void ThenByDescending_NotPipelined_ExtremeComparer(Labeled<ParallelQuery<int>> labeled, int count)
+        {
+            int prev = int.MaxValue;
+            Assert.All(labeled.Item.OrderBy(x => 0).ThenByDescending(x => x, new ExtremeComparer<int>()).ToList(), x => { Assert.InRange(x, int.MinValue, prev); prev = x; });
+        }
+
         // Recursive sort with nested ThenBy...s
         // Due to the use of randomized input, cycles will not start with a known input (and may skip values, etc).
 

src/System.Linq.Parallel/tests/WithCancellationTests.cs

@@ -617,71 +617,5 @@ public static void DontDoWorkIfTokenAlreadyCanceled()
 
             Assert.NotNull(oce);
         }
-
-        // To help the user, we will check if a cancellation token passed to WithCancellation() is
-        // not backed by a disposed CTS.  This will help them identify incorrect cts.Dispose calls, but
-        // doesn't solve all their problems if they don't manage CTS lifetime correctly.
-        // We test via a few random queries that have shown inconsistent behavior in the past.
-        [Fact]
-        public static void PreDisposedCTSPassedToPlinq()
-        {
-            ArgumentException ae1 = null;
-            ArgumentException ae2 = null;
-            ArgumentException ae3 = null;
-
-            CancellationTokenSource cts = new CancellationTokenSource();
-            CancellationToken ct = cts.Token;
-            cts.Dispose();  // Early dispose
-            try
-            {
-                Enumerable.Range(1, 10).AsParallel()
-                    .WithCancellation(ct)
-                    .OrderBy(x => x)
-                    .ToArray();
-            }
-            catch (Exception ex)
-            {
-                // This is not going to be the case since we changed the behavior of WithCancellation
-                // to not throw when called on disposed cancellationtokens.
-                ae1 = (ArgumentException)ex;
-            }
-
-            try
-            {
-                Enumerable.Range(1, 10).AsParallel()
-                    .WithCancellation(ct)
-                    .Last();
-            }
-            catch (Exception ex)
-            {
-                // This is not going to be the case since we changed the behavior of WithCancellation
-                // to not throw when called on disposed cancellationtokens.
-                ae2 = (ArgumentException)ex;
-            }
-
-            try
-            {
-                Enumerable.Range(1, 10).AsParallel()
-                    .WithCancellation(ct)
-                    .OrderBy(x => x)
-                    .Last();
-            }
-            catch (Exception ex)
-            {
-                // This is not going to be the case since we changed the behavior of WithCancellation
-                // to not throw when called on disposed cancellationtokens.
-                ae3 = (ArgumentException)ex;
-            }
-
-            Assert.Null(ae1);
-            Assert.Null(ae2);
-            Assert.Null(ae3);
-        }
-
-        public static void SimulateThreadSleep(int milliseconds)
-        {
-            ManualResetEvent mre = new ManualResetEvent(false);
-            mre.WaitOne(milliseconds);
-        }
     }
 }