Creating a TCP connection using DicomClient.SendAsync() does not take AssociationRequestTimeoutInMs into account

[fredrikcarlbom]

Describe the bug
Creating a TCP connection using DicomClient.SendAsync() does not take AssociationRequestTimeoutInMs into account

To Reproduce
Running the example program takes about 20 seconds in Windows and 130 seconds in Ubuntu (WSL).

Windows 11

dotnet run
Sending 2024-05-03 14:27:41
Done 2024-05-03 14:28:02

Ubuntu 22.04 (WSL)

dotnet run
Sending 05/03/2024 13:36:06
Done 05/03/2024 13:38:15

Expected behavior
Since I set AssociationRequestTimeoutInMs I expected it to take 1 second.

Screenshots or test DICOM files

Same example program as linked above

using FellowOakDicom.Network;
using FellowOakDicom.Network.Client;

var client = DicomClientFactory.Create("1.1.1.1", 4242, false, "dummy", "dummy");
client.ClientOptions.AssociationRequestTimeoutInMs = 1000;

var cEchoRequest = new DicomCEchoRequest {
    OnTimeout = (_, _) => { Console.WriteLine($"TimeOut {DateTime.Now}"); },
    OnResponseReceived = (_, response) => { Console.WriteLine($"Response {DateTime.Now}"); }
};
await client.AddRequestAsync(cEchoRequest);
Console.WriteLine($"Sending {DateTime.Now}");
try {
    await client.SendAsync();
}
catch (AggregateException) {}
Console.WriteLine($"Done {DateTime.Now}");

Environment
Fellow Oak DICOM version: 5.1.2
OS: Windows 11
Platform: .NET 8

[gofal]

Currently, the AssociationRequestTimeoutInMs only is used when the AssociationRequest is sent to the server, after the networkstream has been created successfully.
Creating the networkstream by calling tcpClient.ConnectAsync and tcpClient.GetStream does not have a timeout. So the operating systems default-timeout is applied here.

fo-dicom/FO-DICOM.Core/Network/DesktopNetworkStream.cs

Lines 57 to 59 in f13ce2e

	_tcpClient.ConnectAsync(options.Host, options.Port).Wait();
	Stream stream = _tcpClient.GetStream();

if the AssociationRequestTimeout would also include the creation of the networkstream, then this would be a breaking change. The current default value of 5 seconds might then be too low.
Maybe adding a new Timeout property, which is used for creating a newtwork stream could be introduced. But then the various timeout properties could be confusing for users.
This should be discussed.

[fredrikcarlbom]

Maybe adding a new Timeout property, which is used for creating a newtwork stream could be introduced. But then the various timeout properties could be confusing for users.

I find it somewhat confusing that the AssociationRequestTimeoutInMs doesn't include the original tcp-handshake so I'd argue that introducing an additional configuration could improve how easy it is to understand.

This should be discussed.

Should it be discussed here or as a discussion?

[gofal]

I guess it also might be discussed here. What I meant is, that we should think about pro and cont of including the time, that the creation of the networkstream takes, into the AssociationRequesttimeout.
The DicomClient already has AssociationRequestTimeout, AssociationReleaseTimeout, AssociationLingerTimeout and RequestTimeOut. Is adding one more timeout still fine or already too much? Maybe its fine as long as the name is clear enough.
The name "AssociationRequestTimeout" for example obviously was not clear enough because it might make devs think, that this means the Time until there is an established association.
But sure, if you see, that there are 2 Timeouts, like e.g. NetworkStreamCreationTimeout and AssociationRequestTimeout, then it may become even more clear, that the AssociationRequestTimeout is only about sending the AssociationRequest over an already created network stream while the NetworkStreamCreationTimeout then is responsible for connecting to a network port (including all the ssl handshakes etc)

[amoerie]

Two thoughts here:

1. I also think it makes the most sense to add a NetworkStreamCreationTimeout (I think I prefer something like OpenConnectionTimeout or simply ConnectionTimeout though)
2. Using the advanced API you can already achieve this

// 1 second connection timeout
using var connectionTimeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(1));
var connectionRequest = new AdvancedDicomClientConnectionRequest
{
    //...
};
using var connection = await AdvancedDicomClientConnectionFactory.OpenConnectionAsync(connectionRequest, connectionTimeoutCts.Token);

var openAssociationRequest = new AdvancedDicomClientAssociationRequest
{
//...
};

// 1 second association request timeout
using var associationRequestTimeoutCts = new CancellationTokenSource(TimeSpan.FromSeconds(1));
using var association = await connection.OpenAssociationAsync(openAssociationRequest, associationRequestTimeoutCts.Token);