Add support for per HalibutRuntime timeouts and limits #427

LukeButters · 2023-08-14T05:32:09Z

Background

Statics make testing hard

HalibutLimits are statically set, which means all tests are using the same timeout. This is a problem since either we can have:

High timeouts increasing reliability but with timeout tests taking longer to run.
Low timeouts reducing the time it takes to run timeout test but with decreased reliability of tests.

This introduces a new type HalibutTimeoutsAndLimits, which holds timeouts and limits and is used only in async paths.

The existing HalibutLimits class is marked obsolete.

[SC-45207]

Related to OctopusDeploy/Issues#8266

How to review this PR

Quality ✔️

Pre-requisites

I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
I have considered informing or consulting the right people, according to the ownership map.
I have considered appropriate testing for my change.

shortcut-integration · 2023-08-14T22:08:29Z

This pull request has been linked to Shortcut Story #45207: Improve Halibut to support testing with different TCP timeouts, allowing tests to control timeouts.

LukeButters · 2023-08-14T22:09:18Z

source/Halibut.Tests/Support/LatestClientAndLatestServiceBuilderExtensionMethods.cs

@@ -58,5 +60,15 @@ public static LatestClientAndLatestServiceBuilder WithInstantReconnectPollingRet
        {
            return builder.WithPollingReconnectRetryPolicy(() => new RetryPolicy(1, TimeSpan.Zero, TimeSpan.Zero));
        }
+
+        public static LatestClientAndLatestServiceBuilder WhenTestingAsyncClient(this LatestClientAndLatestServiceBuilder builder, ClientAndServiceTestCase clientAndServiceTestCase, Action<LatestClientAndLatestServiceBuilder> action)


Some of the existing timeout tests could be made faster by making use of this and lowering timeouts.

LukeButters · 2023-08-14T22:09:55Z

source/Halibut.Tests/Support/TestAttributes/LatestClientAndLatestServiceTestCasesAttribute.cs

@@ -23,18 +23,19 @@ public class LatestClientAndLatestServiceTestCasesAttribute : TestCaseSourceAttr
            bool testSyncClients = true,
            bool testAsyncClients = true,
            bool testAsyncServicesAsWell = false, // False means only the sync service will be tested.
+            bool testSyncService = true,


I wonder if these could be in a #define and so defined once but used here and in the below static method. As well as in other builders.

LukeButters · 2023-08-14T22:11:16Z

source/Halibut.Tests/Timeouts/SendingAndReceivingRequestMessagesTimeoutsFixture.cs

+                    .And
+                    .BeLessThan(expectedTimeout + LowerHalibutLimitsForAllTests.HalfTheTcpReceiveTimeout, "We should be timing out on the tcp receive timeout");
+
+                // The polling tentacle, will not reconnect in time since it has a 133s receive control message timeout.


The raises other questions like if the tentacle has sent a large response back, will it be able to move into a control message read/write but timeout on that. Not because the next/proceed took a long time itself but because the data ahead of it resulted in the control messages taking some time.

Is this something we need to look at? If so should we card it?

LukeButters · 2023-08-14T22:13:04Z

source/Halibut/ServiceEndPoint.cs

@@ -8,11 +8,15 @@ public class ServiceEndPoint : IEquatable<ServiceEndPoint>
    {
        readonly string baseUriString;

+        // TODO - ASYNC ME UP!


In another PR we will need a ServiceEndPoint constructed with the halibuttimeoutsandlimits. Or perhaps the halibut runtime should have a ServiceEndPoint factory with timeouts configured from its limits.

Doing something here is captured on the ticket.

LukeButters · 2023-08-14T22:14:15Z

source/Halibut/Transport/Protocol/ControlMessageReader.cs

+
+        CancellationTokenSource GetCancellationTokenSourceFromStreamReadTimeoutAsync(Stream stream)
+        {
+            if (stream.CanTimeout)


TODO mark this as TODO to note we should always be given a stream that can timeout.

WebSockets?

We were not thinking of wrapping that stream in a stream that fakes timing out?

source/Halibut.Tests/Timeouts/SendingAndReceivingRequestMessagesTimeoutsFixture.cs

nathanwoctopusdeploy · 2023-08-14T23:07:07Z

source/Halibut.Tests/Transport/DiscoveryClientFixture.cs

@@ -44,7 +44,7 @@ public async Task DiscoveringNonExistentEndpointThrows(SyncOrAsync syncOrAsync)
 #pragma warning disable CS0612
            await syncOrAsync
                .WhenSync(() => Assert.Throws<HalibutClientException>(() => client.Discover(fakeEndpoint), "No such host is known")).IgnoreResult()
-                .WhenAsync(async () => await AssertAsync.Throws<HalibutClientException>(() => client.DiscoverAsync(fakeEndpoint, CancellationToken), "No such host is known"));
+                .WhenAsync(async () => await AssertAsync.Throws<HalibutClientException>(() => client.DiscoverAsync(fakeEndpoint, new HalibutTimeoutsAndLimits(), CancellationToken), "No such host is known"));


Do we want a helper that sets some good defaults instead of new HalibutTimeoutsAndLimits() so it's consistent with the statics we set at the moment to keep some consistency amount how timeouts are set. Individual tests can choose to change them, otherwise they all get the same.

Ahh, I see they are defaulted from the statics so it is doing that 👍

HalibutTimeoutsAndLimits get its defaults from what is set statically which will be the reduced values as part of LowerHalibutLimits. In theory those should be "good" defaults.

source/Halibut/Diagnostics/HalibutLimits.cs

nathanwoctopusdeploy · 2023-08-14T23:13:00Z

source/Halibut/Diagnostics/HalibutLimits.cs

@@ -107,4 +108,102 @@ public static TimeSpan SafeTcpClientPooledConnectionTimeout
            }
        }
    }
+#pragma warning disable CS0612
+    public class HalibutTimeoutsAndLimits {


Worth adding a unit test for this class to make sure we are setting the properties from the correct statics?

nathanwoctopusdeploy · 2023-08-14T23:15:16Z

source/Halibut/HalibutRuntime.cs

@@ -91,6 +94,7 @@ public HalibutRuntime(IServiceFactory serviceFactory, X509Certificate2 serverCer
            this.messageSerializer = messageSerializer;
            this.pollingReconnectRetryPolicy = pollingReconnectRetryPolicy;
            invoker = new ServiceInvoker(serviceFactory);
+            this.TimeoutsAndLimits = halibutTimeoutsAndLimits;


If a sync Halibut runtime is created, would it make sense to enforce that halibutTimeoutsAndLimits should be null, so the caller knows these are not going to apply and we don't create confusion? It may also help ensure we aren't using them in sync code paths at all as we will get object ref exceptions

Yes and done.

nathanwoctopusdeploy · 2023-08-14T23:15:59Z

source/Halibut/HalibutRuntime.cs

@@ -36,6 +36,7 @@ public class HalibutRuntime : IHalibutRuntime
        readonly ITypeRegistry typeRegistry;
        readonly Lazy<ResponseCache> responseCache = new();
        readonly Func<RetryPolicy> pollingReconnectRetryPolicy;
+        public HalibutTimeoutsAndLimits TimeoutsAndLimits { get; }


Could throw here if it's a sync runtime so callers know they are accessing something that is not supported / being used

nathanwoctopusdeploy · 2023-08-14T23:31:07Z

source/Halibut/Transport/Protocol/WebSocketStream.cs

@@ -50,7 +50,11 @@ public async Task<string> ReadTextMessage()
            var buffer = new ArraySegment<byte>(new byte[10000]);
            while (true)
            {
+                // TODO - ASYNC ME UP!
+                // What should the timeout be here? thew new code that allows passing in a timeout or the static value?


#414 changes this stream to add async methods

nathanwoctopusdeploy · 2023-08-14T23:32:10Z

source/Halibut/Transport/SecureConnection.cs

@@ -30,7 +30,11 @@ public void NotifyUsed()

        public bool HasExpired()
        {
+            // TODO - ASYNC ME UP!
+            // Use the HalibutRuntimeLimits


Is there a reason some changes are being split out of this pr?

nathanwoctopusdeploy · 2023-08-14T23:34:57Z

source/Halibut/Transport/TcpConnectionFactory.cs

@@ -47,12 +47,12 @@ public IConnection EstablishNewConnection(ExchangeProtocolBuilder exchangeProtoc
            return new SecureConnection(client, ssl, exchangeProtocolBuilder, log);
        }

-        public async Task<IConnection> EstablishNewConnectionAsync(ExchangeProtocolBuilder exchangeProtocolBuilder, ServiceEndPoint serviceEndpoint, ILog log, CancellationToken cancellationToken)
+        public async Task<IConnection> EstablishNewConnectionAsync(ExchangeProtocolBuilder exchangeProtocolBuilder, ServiceEndPoint serviceEndpoint, HalibutTimeoutsAndLimits halibutTimeoutsAndLimits, ILog log, CancellationToken cancellationToken)


Given the limits are set on the runtime, should they be given to the constructor of the factory and then used in the methods so wee don't have to pass them around to all methods

May mean some methods below would benefit from not being static

nathanwoctopusdeploy

Nice, a few suggestions. Nice approach marking the old limits as obsolete so you get compile time errors where they are being used 👍

LukeButters force-pushed the sast/per-halibutruntime-timeouts branch from 95987c3 to 3343ea3 Compare August 14, 2023 22:08

LukeButters commented Aug 14, 2023

View reviewed changes

LukeButters changed the title ~~Add support for per HalibutRuntime limits~~ Add support for per HalibutRuntime timeouts and limits Aug 14, 2023

LukeButters marked this pull request as ready for review August 14, 2023 22:15

LukeButters requested a review from a team as a code owner August 14, 2023 22:15

nathanwoctopusdeploy reviewed Aug 14, 2023

View reviewed changes

source/Halibut.Tests/Timeouts/SendingAndReceivingRequestMessagesTimeoutsFixture.cs Outdated Show resolved Hide resolved

nathanwoctopusdeploy reviewed Aug 14, 2023

View reviewed changes

source/Halibut/Diagnostics/HalibutLimits.cs Outdated Show resolved Hide resolved

nathanwoctopusdeploy reviewed Aug 14, 2023

View reviewed changes

source/Halibut/Diagnostics/HalibutLimits.cs Outdated Show resolved Hide resolved

nathanwoctopusdeploy reviewed Aug 14, 2023

View reviewed changes

nathanwoctopusdeploy approved these changes Aug 14, 2023

View reviewed changes

nathanwoctopusdeploy requested review from acodrington and sburmanoctopus August 14, 2023 23:41

Add support for per HalibutRuntime limits

06b298a

LukeButters force-pushed the sast/per-halibutruntime-timeouts branch from c6e7d14 to 06b298a Compare August 15, 2023 03:13

LukeButters enabled auto-merge (squash) August 15, 2023 03:14

.

167d835

LukeButters merged commit eab6dd8 into main Aug 15, 2023
10 of 14 checks passed

LukeButters deleted the sast/per-halibutruntime-timeouts branch August 15, 2023 05:04

LukeButters mentioned this pull request Aug 15, 2023

Support HalibutTimeoutsAndLimits in SecureConnection #430

Merged

3 tasks

LukeButters mentioned this pull request Aug 15, 2023

ServiceEndpoint can now use HalibutTimeoutsAndLimits #432

Merged

3 tasks

acodrington mentioned this pull request Aug 17, 2023

Make WebSocketStream async #414

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for per HalibutRuntime timeouts and limits #427

Add support for per HalibutRuntime timeouts and limits #427

LukeButters commented Aug 14, 2023 •

edited by nathanwoctopusdeploy

Loading

shortcut-integration bot commented Aug 14, 2023

LukeButters Aug 14, 2023

LukeButters Aug 14, 2023

LukeButters Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

LukeButters Aug 14, 2023 •

edited

Loading

LukeButters Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

LukeButters Aug 15, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

LukeButters Aug 15, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023 •

edited

Loading

LukeButters Aug 15, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy Aug 14, 2023

nathanwoctopusdeploy left a comment

Add support for per HalibutRuntime timeouts and limits #427

Add support for per HalibutRuntime timeouts and limits #427

Conversation

LukeButters commented Aug 14, 2023 • edited by nathanwoctopusdeploy Loading

Background

How to review this PR

Pre-requisites

shortcut-integration bot commented Aug 14, 2023

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

LukeButters Aug 14, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanwoctopusdeploy Aug 14, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanwoctopusdeploy left a comment

Choose a reason for hiding this comment

LukeButters commented Aug 14, 2023 •

edited by nathanwoctopusdeploy

Loading

LukeButters Aug 14, 2023 •

edited

Loading

nathanwoctopusdeploy Aug 14, 2023 •

edited

Loading