Update S7Connection.cs

[mach-sfortier]

Modification to set a timeout for the OpenAsync

I noticed that this function takes a while when the destination IP address is not reachable. I think it will be nice if we can set a Timeout parameter for this function.

[mycroes]

Hi @mach-sfortier,

I'd rather see this changed to add an optional CancellationToken to the OpenAsync method. Optionally an overload with a timeout could be available, but most importantly I'd rather have the implementation use the cooperative cancellation that's available in other places in .NET (and actually seems to be available on .NET 5 for TcpClient.OpenAsync). Feel free to update your PR to use a CancellationToken (as optional argument) instead, otherwise we can leave this PR open as reminder 😄

Regards, Michael

[mach-sfortier]

Hi @mycroes,

I sent changes where the ConnectAsync has now an overload method with a timeout parameter.

For the optionnal CancellationToken parameter, I tried something with an extension method to stay compatible with the .Net Framework and .Net Standard but it wasn't working.

Did you have any intention to port your project to .Net 5?

Best regards, Stephane

[scamille]

https://stackoverflow.com/a/22078975 Is a solution which would incorporate cancellation token.

Sally7 should work fine on NET5 through its Netsstandard target. There is currently no need to target NET5.0 specifically, as far as I know, in particular if framework support is still on the table.

[mach-sfortier]

Hi @scamille,

You are right. It will be more comprehensive with the use of the TimeSpan. Thanks! I already sent the changes.

For the CancellationToken, it wasn't for the Task.Delay that I thought it should be used but more for the OpenAsync. It's already done in .Net 5.0 but not in .net Standard and .Net Framework.

I found this example :

public static class Extensions
    {
        public static async Task ConnectAsync(this TcpClient tcpClient, string host, int port, CancellationToken cancellationToken) {
            if (tcpClient == null) {
                throw new ArgumentNullException(nameof(tcpClient));
            }

            cancellationToken.ThrowIfCancellationRequested();

            using (cancellationToken.Register(() => tcpClient.Close())) {
                try {
                    cancellationToken.ThrowIfCancellationRequested();

                    await tcpClient.ConnectAsync(host, port).ConfigureAwait(false);
                } catch (NullReferenceException) when (cancellationToken.IsCancellationRequested) {
                    cancellationToken.ThrowIfCancellationRequested();
                }
            }
        }
    }

I tried it and I always got a NullReferenceException for the EndConnect of TcpClient when I try to cancel the OpenAsync with the CancellationToken.

[scamille]

I always thought that cancelling the connect on older framework just does nothing. If it causes an exception, that is indeed bad, and means the cancellation token should not be passed to the function.

I think I would only offer a connect function overload with cancellation token for net5+ then. Sally7 would need a separate net5 target so that net5+ clients can take advantage of this.

[mycroes]

This is now handled in #30, so I'm closing this one.