Unverified Commit cb2f1559 authored by Nick Craver's avatar Nick Craver Committed by GitHub

This removes TaskSource.CreateDenyExecSync (#798)

After 50547eba, TaskSource.Create and TaskSource.CreateDenyExecSync methods are the same. There was some crud left everywhere that appeared like differences but it's really determined once in the static initializer for TaskSource based on the platform anyway.

I've fixed the tests as well - they were a bit backwards on logic/assumptions so hard to read, and have been failing for some time. Now we'd expect ContinueWithExecSync to *not* be hijacked, and everything else to be...that's how the tests are as well.

Note: the #if DEBUG change is because internals are already visible to tests...just simpler.
parent aaa5b514
...@@ -33,24 +33,19 @@ private static TaskCompletionSource<T> Create<T>(SourceOrign origin) ...@@ -33,24 +33,19 @@ private static TaskCompletionSource<T> Create<T>(SourceOrign origin)
{ {
case SourceOrign.NewTCS: return new TaskCompletionSource<T>(); case SourceOrign.NewTCS: return new TaskCompletionSource<T>();
case SourceOrign.Create: return TaskSource.Create<T>(null); case SourceOrign.Create: return TaskSource.Create<T>(null);
case SourceOrign.CreateDenyExec: return TaskSource.CreateDenyExecSync<T>(null);
default: throw new ArgumentOutOfRangeException(nameof(origin)); default: throw new ArgumentOutOfRangeException(nameof(origin));
} }
} }
[Theory] [Theory]
// regular framework behaviour: 2 out of 3 cause hijack // regular framework behaviour: 2 out of 3 cause hijack
[InlineData(SourceOrign.NewTCS, AttachMode.ContinueWith, false)] [InlineData(SourceOrign.NewTCS, AttachMode.ContinueWith, true)]
[InlineData(SourceOrign.NewTCS, AttachMode.ContinueWithExecSync, true)] [InlineData(SourceOrign.NewTCS, AttachMode.ContinueWithExecSync, false)]
[InlineData(SourceOrign.NewTCS, AttachMode.Await, true)] [InlineData(SourceOrign.NewTCS, AttachMode.Await, true)]
// Create is just a wrapper of ^^^; expect the same // Create is just a wrapper of ^^^; expect the same
[InlineData(SourceOrign.Create, AttachMode.ContinueWith, false)] [InlineData(SourceOrign.Create, AttachMode.ContinueWith, true)]
[InlineData(SourceOrign.Create, AttachMode.ContinueWithExecSync, true)] [InlineData(SourceOrign.Create, AttachMode.ContinueWithExecSync, false)]
[InlineData(SourceOrign.Create, AttachMode.Await, true)] [InlineData(SourceOrign.Create, AttachMode.Await, true)]
// deny exec-sync: none should cause hijack
[InlineData(SourceOrign.CreateDenyExec, AttachMode.ContinueWith, false)]
[InlineData(SourceOrign.CreateDenyExec, AttachMode.ContinueWithExecSync, false)]
[InlineData(SourceOrign.CreateDenyExec, AttachMode.Await, false)]
public void TestContinuationHijacking(SourceOrign origin, AttachMode attachMode, bool expectHijack) public void TestContinuationHijacking(SourceOrign origin, AttachMode attachMode, bool expectHijack)
{ {
TaskCompletionSource<int> source = Create<int>(origin); TaskCompletionSource<int> source = Create<int>(origin);
...@@ -64,19 +59,18 @@ public void TestContinuationHijacking(SourceOrign origin, AttachMode attachMode, ...@@ -64,19 +59,18 @@ public void TestContinuationHijacking(SourceOrign origin, AttachMode attachMode,
Assert.NotEqual(-1, from); // not set Assert.NotEqual(-1, from); // not set
if (expectHijack) if (expectHijack)
{ {
Assert.True(settingThread == from, "expected hijack; didn't happen"); Assert.True(settingThread != from, $"expected hijack; didn't happen, Origin={settingThread}, Final={from}");
} }
else else
{ {
Assert.False(settingThread == from, "setter was hijacked"); Assert.True(settingThread == from, $"setter was hijacked, Origin={settingThread}, Final={from}");
} }
} }
public enum SourceOrign public enum SourceOrign
{ {
NewTCS, NewTCS,
Create, Create
CreateDenyExec
} }
public enum AttachMode public enum AttachMode
......
...@@ -1967,7 +1967,7 @@ internal Task<T> ExecuteAsyncImpl<T>(Message message, ResultProcessor<T> process ...@@ -1967,7 +1967,7 @@ internal Task<T> ExecuteAsyncImpl<T>(Message message, ResultProcessor<T> process
} }
else else
{ {
var tcs = TaskSource.CreateDenyExecSync<T>(state); var tcs = TaskSource.Create<T>(state);
var source = ResultBox<T>.Get(tcs); var source = ResultBox<T>.Get(tcs);
if (!TryPushMessageToBridge(message, processor, source, ref server)) if (!TryPushMessageToBridge(message, processor, source, ref server))
{ {
......
...@@ -78,7 +78,7 @@ internal override Task<T> ExecuteAsync<T>(Message message, ResultProcessor<T> pr ...@@ -78,7 +78,7 @@ internal override Task<T> ExecuteAsync<T>(Message message, ResultProcessor<T> pr
} }
else else
{ {
var tcs = TaskSource.CreateDenyExecSync<T>(asyncState); var tcs = TaskSource.Create<T>(asyncState);
var source = ResultBox<T>.Get(tcs); var source = ResultBox<T>.Get(tcs);
message.SetSource(source, processor); message.SetSource(source, processor);
task = tcs.Task; task = tcs.Task;
......
...@@ -72,7 +72,7 @@ internal override Task<T> ExecuteAsync<T>(Message message, ResultProcessor<T> pr ...@@ -72,7 +72,7 @@ internal override Task<T> ExecuteAsync<T>(Message message, ResultProcessor<T> pr
} }
else else
{ {
var tcs = TaskSource.CreateDenyExecSync<T>(asyncState); var tcs = TaskSource.Create<T>(asyncState);
var source = ResultBox<T>.Get(tcs); var source = ResultBox<T>.Get(tcs);
message.SetSource(source, processor); message.SetSource(source, processor);
task = tcs.Task; task = tcs.Task;
......
...@@ -562,7 +562,7 @@ internal void OnHeartbeat() ...@@ -562,7 +562,7 @@ internal void OnHeartbeat()
internal Task<T> QueueDirectAsync<T>(Message message, ResultProcessor<T> processor, object asyncState = null, PhysicalBridge bridge = null) internal Task<T> QueueDirectAsync<T>(Message message, ResultProcessor<T> processor, object asyncState = null, PhysicalBridge bridge = null)
{ {
var tcs = TaskSource.CreateDenyExecSync<T>(asyncState); var tcs = TaskSource.Create<T>(asyncState);
var source = ResultBox<T>.Get(tcs); var source = ResultBox<T>.Get(tcs);
message.SetSource(processor, source); message.SetSource(processor, source);
if (bridge == null) bridge = GetBridge(message.Command); if (bridge == null) bridge = GetBridge(message.Command);
......
...@@ -13,10 +13,7 @@ namespace StackExchange.Redis ...@@ -13,10 +13,7 @@ namespace StackExchange.Redis
/// see https://stackoverflow.com/a/22588431/23354 for more information; a huge /// see https://stackoverflow.com/a/22588431/23354 for more information; a huge
/// thanks to Eli Arbel for spotting this (even though it is pure evil; it is *my kind of evil*) /// thanks to Eli Arbel for spotting this (even though it is pure evil; it is *my kind of evil*)
/// </summary> /// </summary>
#if DEBUG internal static class TaskSource
public // for the unit tests in TaskTests.cs
#endif
static class TaskSource
{ {
#if !PLAT_SAFE_CONTINUATIONS #if !PLAT_SAFE_CONTINUATIONS
// on .NET < 4.6, it was possible to have threads hijacked; this is no longer a problem in 4.6 and core-clr 5, // on .NET < 4.6, it was possible to have threads hijacked; this is no longer a problem in 4.6 and core-clr 5,
...@@ -93,15 +90,5 @@ public static TaskCompletionSource<T> Create<T>(object asyncState) ...@@ -93,15 +90,5 @@ public static TaskCompletionSource<T> Create<T>(object asyncState)
return new TaskCompletionSource<T>(asyncState, TaskCreationOptions.None); return new TaskCompletionSource<T>(asyncState, TaskCreationOptions.None);
#endif #endif
} }
/// <summary>
/// Create a new TaskCompletionSource that will not allow result-setting threads to be hijacked
/// </summary>
public static TaskCompletionSource<T> CreateDenyExecSync<T>(object asyncState)
{
var source = new TaskCompletionSource<T>(asyncState);
//DenyExecSync(source.Task);
return source;
}
} }
} }
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment