Skip to content

allow fifo task execution #49

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zachrybaker wants to merge 2 commits into
base: master
Choose a base branch
from
Open

allow fifo task execution #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd44e21
allow fifo task execution
Sep 5, 2023
b5f49a6
listener pool control
Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions GrpcDotNetNamedPipes.PerfTests/GrpcDotNetNamedPipes.PerfTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@

<ItemGroup>
<PackageReference Include="Grpc.AspNetCore" Version="2.49.0" Condition="'$(TargetFramework)' != 'net462'" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.2" />
Copy link
Owner

@cyanfish cyanfish Sep 8, 2023

Choose a reason for hiding this comment

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

Let's leave package version updates out of this PR unless they're needed for some reason

<PackageReference Include="xunit.runner.visualstudio" Version="2.5.0">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>

<ItemGroup>
Expand Down
17 changes: 10 additions & 7 deletions GrpcDotNetNamedPipes.Tests/GrpcDotNetNamedPipes.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,17 @@
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Google.Protobuf" Version="3.21.9" />
<PackageReference Include="Grpc" Version="2.46.5" />
<PackageReference Include="Grpc.Core.Api" Version="2.49.0" />
<PackageReference Include="Grpc.Tools" Version="2.50.0" PrivateAssets="All" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.3.2" />
<PackageReference Include="Google.Protobuf" Version="3.24.3" />
<PackageReference Include="Grpc" Version="2.46.6" />
<PackageReference Include="Grpc.Core.Api" Version="2.57.0" />
<PackageReference Include="Grpc.Tools" Version="2.58.0" PrivateAssets="All" />
<PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.2" />
<PackageReference Include="System.IO.Pipes.AccessControl" Version="5.0.0" />
<PackageReference Include="xunit" Version="2.4.1" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.4.1" />
<PackageReference Include="xunit" Version="2.5.0" />
<PackageReference Include="xunit.runner.visualstudio" Version="2.5.0">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>

<ItemGroup>
Expand Down
12 changes: 8 additions & 4 deletions GrpcDotNetNamedPipes/Internal/ServerConnectionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ internal class ServerConnectionContext : TransportMessageHandler, IDisposable
private readonly ConnectionLogger _logger;
private readonly Dictionary<string, Func<ServerConnectionContext, Task>> _methodHandlers;
private readonly PayloadQueue _payloadQueue;

private readonly TaskFactory _taskFactory;

public ServerConnectionContext(NamedPipeServerStream pipeStream, ConnectionLogger logger,
Dictionary<string, Func<ServerConnectionContext, Task>> methodHandlers)
Dictionary<string, Func<ServerConnectionContext, Task>> methodHandlers, TaskFactory taskFactory)
{
CallContext = new NamedPipeCallContext(this);
PipeStream = pipeStream;
Expand All @@ -32,6 +33,7 @@ public ServerConnectionContext(NamedPipeServerStream pipeStream, ConnectionLogge
_methodHandlers = methodHandlers;
_payloadQueue = new PayloadQueue();
CancellationTokenSource = new CancellationTokenSource();
_taskFactory = taskFactory;
}

public NamedPipeServerStream PipeStream { get; }
Expand All @@ -58,11 +60,13 @@ public IServerStreamWriter<TResponse> CreateResponseStream<TResponse>(Marshaller
return new ResponseStreamWriterImpl<TResponse>(Transport, CancellationToken.None, responseMarshaller,
() => IsCompleted);
}

public override void HandleRequestInit(string methodFullName, DateTime? deadline)
{
Deadline = new Deadline(deadline);
Task.Run(async () => await _methodHandlers[methodFullName](this).ConfigureAwait(false));
// Note the use of .ConfigureAwait(false) here...
// https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html
_taskFactory.StartNew(async () => await _methodHandlers[methodFullName](this).ConfigureAwait(false));
}

public override void HandleHeaders(Metadata headers) => RequestHeaders = headers;
Expand Down
7 changes: 5 additions & 2 deletions GrpcDotNetNamedPipes/Internal/ServerStreamPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ namespace GrpcDotNetNamedPipes.Internal;

internal class ServerStreamPool : IDisposable
{
private const int PoolSize = 4;
private readonly int PoolSize = 4;
private const int FallbackMin = 100;
private const int FallbackMax = 10_000;

Expand All @@ -37,6 +37,8 @@ public ServerStreamPool(string pipeName, NamedPipeServerOptions options,
_options = options;
_handleConnection = handleConnection;
_invokeError = invokeError;
if (options.ThreadPoolSize > 0)
PoolSize = options.ThreadPoolSize;
}

private NamedPipeServerStream CreatePipeServer()
Expand Down Expand Up @@ -161,7 +163,8 @@ private void RunHandleConnection(NamedPipeServerStream pipeServer)
try
{
await _handleConnection(pipeServer);
pipeServer.Disconnect();
if (pipeServer.IsConnected)
pipeServer.Disconnect();
}
catch (Exception error)
{
Expand Down
6 changes: 4 additions & 2 deletions GrpcDotNetNamedPipes/NamedPipeServer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace GrpcDotNetNamedPipes;
public class NamedPipeServer : IDisposable
{
private readonly ServerStreamPool _pool;
private readonly TaskFactory _taskFactory;
private readonly Action<string> _log;
private readonly Dictionary<string, Func<ServerConnectionContext, Task>> _methodHandlers = new();

Expand All @@ -32,11 +33,12 @@ public NamedPipeServer(string pipeName, NamedPipeServerOptions options)
{
}

internal NamedPipeServer(string pipeName, NamedPipeServerOptions options, Action<string> log)
public NamedPipeServer(string pipeName, NamedPipeServerOptions options, Action<string> log)
Copy link
Owner

@cyanfish cyanfish Sep 5, 2023

Choose a reason for hiding this comment

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

This should still be internal

{
_pool = new ServerStreamPool(pipeName, options, HandleConnection, InvokeError);
_log = log;
ServiceBinder = new ServiceBinderImpl(this);
_taskFactory = options.TaskFactory ?? new TaskFactory();
}

public ServiceBinderBase ServiceBinder { get; }
Expand Down Expand Up @@ -66,7 +68,7 @@ public void Dispose()
private async Task HandleConnection(NamedPipeServerStream pipeStream)
{
var logger = ConnectionLogger.Server(_log);
var ctx = new ServerConnectionContext(pipeStream, logger, _methodHandlers);
var ctx = new ServerConnectionContext(pipeStream, logger, _methodHandlers, _taskFactory);
await Task.Run(new PipeReader(pipeStream, ctx, logger, ctx.Dispose, InvokeError).ReadLoop);
}

Expand Down
13 changes: 13 additions & 0 deletions GrpcDotNetNamedPipes/NamedPipeServerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,17 @@ public class NamedPipeServerOptions
/// </summary>
public PipeSecurity PipeSecurity { get; set; }
#endif

/// <summary>
/// Gets or sets a Custom Task Factory to control how tasks are serviced.
/// For example, causing threads to be processsed in FIFO rather than LIFO
/// by using TaskCreationOptions.preferFairness
/// </summary>
public TaskFactory TaskFactory { get; set; }

/// <summary>
/// Gets or sets a count of threads to use for the listener.
/// If you need to address a synchronous code execution issue, try increasing
/// </summary>
public int ThreadPoolSize { get; set; } = 4;
Copy link
Owner

@cyanfish cyanfish Sep 8, 2023

Choose a reason for hiding this comment

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

Are you sure this helps? In general the listener thread pool is sitting idle and whenever a connection is established, immediately hands off to a task on a different thread and goes back to idling. You shouldn't need a ton of threads to be able to handle that.

If there is a problem it's presumably in high-contention scenarios where clients have trouble connecting, but in that scenario I would rather make improvements to client connection reliability that work out of the box rather than adding a knob that needs to be tuned.

}