Skip to content
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.

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

Sast/reduce the authentication and long poll tcp timeouts luke #545

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 12 additions & 5 deletions source/Halibut.Tests/HalibutTimeoutsAndLimitsForTestsBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ namespace Halibut.Tests
public class HalibutTimeoutsAndLimitsForTestsBuilder
{
public static readonly TimeSpan HalfTheTcpReceiveTimeout = TimeSpan.FromSeconds(22.5);
static readonly TimeSpan PollingQueueWaitTimeout = TimeSpan.FromSeconds(20);
static readonly TimeSpan ShortTimeout = TimeSpan.FromSeconds(15);

public HalibutTimeoutsAndLimits Build()
{
Expand All @@ -22,13 +24,18 @@ public HalibutTimeoutsAndLimits Build()
TcpClientTimeout = new(
sendTimeout: HalfTheTcpReceiveTimeout + HalfTheTcpReceiveTimeout,
receiveTimeout: HalfTheTcpReceiveTimeout + HalfTheTcpReceiveTimeout),

TcpClientHeartbeatTimeout = new(
sendTimeout: TimeSpan.FromSeconds(15),
receiveTimeout: TimeSpan.FromSeconds(15)),

TcpClientHeartbeatSendTimeout = ShortTimeout,
TcpClientHeartbeatReceiveTimeout = ShortTimeout,

TcpClientAuthenticationSendTimeout = ShortTimeout,
TcpClientAuthenticationReceiveTimeout = ShortTimeout,

TcpClientPollingForNextRequestSendTimeout = ShortTimeout,
TcpClientPollingForNextRequestReceiveTimeout = PollingQueueWaitTimeout + ShortTimeout,

TcpClientConnectTimeout = TimeSpan.FromSeconds(20),
PollingQueueWaitTimeout = TimeSpan.FromSeconds(20)
PollingQueueWaitTimeout = PollingQueueWaitTimeout
};
}
}
Expand Down
12 changes: 6 additions & 6 deletions source/Halibut.Tests/Support/SerilogLoggerBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,18 +57,18 @@ public ILogger Build()
var testHash = CurrentTestHash();
var logger = Logger.ForContext("TestHash", testHash);

if (!HasLoggedTestHash.Contains(testName))
{
HasLoggedTestHash.Add(testName);
logger.Information($"Test: {TestContext.CurrentContext.Test.Name} has hash {testHash}");
}

if (traceFileLogger != null)
{
TraceLoggers.AddOrUpdate(testName, traceFileLogger, (_, _) => throw new Exception("This should never be updated. If it is, it means that a test is being run multiple times in a single test run"));
traceFileLogger.SetTestHash(testHash);
}

if (!HasLoggedTestHash.Contains(testName))
{
HasLoggedTestHash.Add(testName);
logger.Information($"Test: {TestContext.CurrentContext.Test.Name} has hash {testHash}");
}

return logger;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ int writeNumberToPauseOn // Ie pause on the first or second write
}

sw.Stop();
sw.Elapsed.Should().BeCloseTo(clientAndService.Service.TimeoutsAndLimits.TcpClientTimeout.ReceiveTimeout, TimeSpan.FromSeconds(15), "Since a paused connection early on should not hang forever.");
sw.Elapsed.Should().BeCloseTo(clientAndService.Service.TimeoutsAndLimits.TcpClientAuthenticationSendTimeout, TimeSpan.FromSeconds(15), "Since a paused connection early on should not hang forever.");

await echo.SayHelloAsync("The pump wont be paused here so this should work.");
}
Expand Down
55 changes: 48 additions & 7 deletions source/Halibut.Tests/Transport/Protocol/ProtocolFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,22 +121,32 @@ public async Task ShouldExchangeAsSubscriber()
AssertOutput(@"
--> MX-SUBSCRIBE subscriptionId
<-- MX-SERVER
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED");
}
Expand Down Expand Up @@ -179,37 +189,57 @@ public async Task ShouldExchangeAsSubscriberWithPooling()
AssertOutput(@"
--> MX-SUBSCRIBE subscriptionId
<-- MX-SERVER
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> ResponseMessage
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED
|-- Set Timeout PollingForNextRequestShortTimeout
<-- RequestMessage
|-- Revert Timeout PollingForNextRequestShortTimeout
--> NEXT
<-- PROCEED");
}
Expand Down Expand Up @@ -279,17 +309,12 @@ public void SetNumberOfReads(int reads)
numberOfReads = reads;
}

public void IdentifyAsClient()
{
output.AppendLine("--> MX-CLIENT");
output.AppendLine("<-- MX-SERVER");
}

public async Task IdentifyAsClientAsync(CancellationToken cancellationToken)
{
await Task.CompletedTask;

IdentifyAsClient();
output.AppendLine("--> MX-CLIENT");
output.AppendLine("<-- MX-SERVER");
}

public async Task SendNextAsync(CancellationToken cancellationToken)
Expand Down Expand Up @@ -371,6 +396,22 @@ public async Task<T> ReceiveAsync<T>(CancellationToken cancellationToken)
return (T)(nextReadQueue.Count > 0 ? nextReadQueue.Dequeue() : default(T));
}

public async Task WithTimeout(MessageExchangeStreamTimeout timeout, Func<Task> func)
{
output.AppendLine("|-- Set Timeout " + timeout);
await func();
output.AppendLine("|-- Revert Timeout " + timeout);
}

public async Task<T> WithTimeout<T>(MessageExchangeStreamTimeout timeout, Func<Task<T>> func)
{
output.AppendLine("|-- Set Timeout " + timeout);
var response = await func();
output.AppendLine("|-- Revert Timeout " + timeout);

return response;
}

public override string ToString()
{
return output.ToString();
Expand Down
26 changes: 17 additions & 9 deletions source/Halibut/Diagnostics/HalibutTimeoutsAndLimits.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,25 @@ public HalibutTimeoutsAndLimits() { }
/// <summary>
/// Amount of time to wait for a TCP or SslStream read/write to complete successfully
/// </summary>
public SendReceiveTimeout TcpClientTimeout { get; set; } = new(sendTimeout: TimeSpan.FromMinutes(10), receiveTimeout: TimeSpan.FromMinutes(10));
public TimeSpan TcpClientSendTimeout { get; set; } = TimeSpan.FromMinutes(1);

/// <summary>
/// Amount of time a connection can stay in the pool
/// Amount of time to wait for a TCP or SslStream read to complete successfully
/// </summary>
public TimeSpan TcpClientPooledConnectionTimeout { get; set; } = TimeSpan.FromMinutes(9);
public TimeSpan TcpClientReceiveTimeout { get; set; } = TimeSpan.FromMinutes(5);

/// <summary>
/// Amount of time to wait for a TCP or SslStream read/write to complete successfully for a control message
/// Amount of time a connection can stay in the pool
/// </summary>
public SendReceiveTimeout TcpClientHeartbeatTimeout { get; set; } = new(sendTimeout: TimeSpan.FromSeconds(60), receiveTimeout: TimeSpan.FromSeconds(60));
public TimeSpan TcpClientPooledConnectionTimeout { get; set; } = TimeSpan.FromMinutes(4.5);

public TimeSpan TcpClientHeartbeatSendTimeout { get; set; } = TimeSpan.FromSeconds(60);
public TimeSpan TcpClientHeartbeatReceiveTimeout { get; set; } = TimeSpan.FromSeconds(60);

public TimeSpan TcpClientAuthenticationSendTimeout { get; set; } = TimeSpan.FromSeconds(60);
public TimeSpan TcpClientAuthenticationReceiveTimeout { get; set; } = TimeSpan.FromSeconds(60);
public TimeSpan TcpClientPollingForNextRequestSendTimeout { get; set; } = TimeSpan.FromSeconds(60);
public TimeSpan TcpClientPollingForNextRequestReceiveTimeout { get; set; } = TimeSpan.FromSeconds(60);

/// <summary>
/// Amount of time to wait for a successful TCP or WSS connection
Expand All @@ -82,13 +90,13 @@ public TimeSpan SafeTcpClientPooledConnectionTimeout
{
get
{
if (TcpClientPooledConnectionTimeout < TcpClientTimeout.ReceiveTimeout)
if (TcpClientPooledConnectionTimeout < TcpClientReceiveTimeout)
{
return TcpClientPooledConnectionTimeout;
}

var timeout = TcpClientTimeout.ReceiveTimeout - TimeSpan.FromSeconds(10);
return timeout > TimeSpan.Zero ? timeout : TcpClientTimeout.ReceiveTimeout;
var timeout = TcpClientReceiveTimeout - TimeSpan.FromSeconds(10);
return timeout > TimeSpan.Zero ? timeout : TcpClientReceiveTimeout;
}
}

Expand Down
3 changes: 3 additions & 0 deletions source/Halibut/Transport/Protocol/IMessageExchangeStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,8 @@ public interface IMessageExchangeStream
Task SendAsync<T>(T message, CancellationToken cancellationToken);

Task<T> ReceiveAsync<T>(CancellationToken cancellationToken);

Task WithTimeout(MessageExchangeStreamTimeout timeout, Func<Task> func);
Task<T> WithTimeout<T>(MessageExchangeStreamTimeout timeout, Func<Task<T>> func);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,9 @@ public async Task ExchangeAsSubscriberAsync(Uri subscriptionId, Func<RequestMess

static async Task ReceiveAndProcessRequestAsync(IMessageExchangeStream stream, Func<RequestMessage, Task<ResponseMessage>> incomingRequestProcessor, CancellationToken cancellationToken)
{
var request = await stream.ReceiveAsync<RequestMessage>(cancellationToken);
var request = await stream.WithTimeout(
MessageExchangeStreamTimeout.PollingForNextRequestShortTimeout,
async () => await stream.ReceiveAsync<RequestMessage>(cancellationToken));

if (request != null)
{
Expand Down
Loading