Am I closing the TCP Client efficiently?

Question

TcpClient tcp = new TcpClient();
bool failed = false;
IAsyncResult connection = tcp.BeginConnect(host, port, null, null);
if (!connection.AsyncWaitHandle.WaitOne(_connectTimeout)) 
{
    failed = true;
    if (tcp.Client.Connected) 
    {
        tcp.GetStream().Close();
        tcp.Close();
    }
} 
else 
{
    if (!tcp.Connected) 
    {
        failed = true;
        if (tcp.Client.Connected) 
        {
            tcp.GetStream().Close();
        }
        tcp.Close();
    }
}
return tcp;

The code above is what I call to connect to a host, port of a proxy. The WaitOne is essentially a timeout. If it returns false, it's timed out.

My question here, is am I calling Close/Dispose/GetStream().Close etc properly on each condition? From what I can tell I should be using EndConnect here with the connection variable but wherever I try to place it, it gives me a SocketException saying the target machine refused connection, yet its either not connected anyway or it IS connected already.

Every call to a `BeginX` operation must always be paired with a call to the corresponding `EndX` operation (unless the `Begin` itself throws). The `End` may throw, but that's just the result of your asynchronous operation, and should be handled with an exception handler just as you'd do in the synchronous case. Failure to do this can cause resource leaks, and in this case *will* cause resource leaks, since the event behind `AsyncWaitHandle` can't be released until it's called. [See also](https://stackoverflow.com/q/17118632/4137916), but note that the accepted answer actually gets it wrong. — Jeroen Mostert, May 04 '18 at 10:27
@JeroenMostert I see, so you think I should EndX before returning but also add an exception handler to the EndX code? — Ma Dude, May 04 '18 at 14:16
The main problem with calling `EndConnect` is that it will block and wait for the operation to succeed or fail, if it hasn't already. This ruins the idea of having a timeout for the whole thing. One of the answers on the linked question should be the proper way to handle the fact that there's no timeout parameter on the `Connect` operation itself, but I'm not sure which one. :-P — Jeroen Mostert, May 04 '18 at 14:22
@JeroenMostert https://stackoverflow.com/a/26261886/9041707 this answer seems to be the perfect one. I edited up a bit to not use a custom state class, completely removed the need for state.success and moved the EndConnect function to an inline arrow function. After that, it works perfect and kinda explained to me how its callback actually works to the point I understand whats happening. Theres no longer any overflowing exceptions and CPU seems to not be as high, but it could be improved. Only thing is, im confused on why it doesnt .Close before return client or if I need to. — Ma Dude, May 04 '18 at 16:05
Oh, of course. Duh. I was so fixated on your synchronous use that I didn't consider the more common use case, which is actually passing a callback, and can be combined with waiting on an event. (It does call `.Close`, in the `End` callback, which is always called. It doesn't call `.Close` on the success path, of course, since you still have to use the client!) — Jeroen Mostert, May 04 '18 at 16:11
@JeroenMostert Oh right haha. Thanks!. I might aswell post up my edited version and mark you as credit. — Ma Dude, May 04 '18 at 16:20

score 0 · Answer 1 · answered May 04 '18 at 16:22

This resolved my question (It's an edit by me of another solution, credit below):

TcpClient tcp = new TcpClient();
#region Try connect
IAsyncResult ar = tcp.BeginConnect(host, port, (ari) => {
    TcpClient tcpi = (TcpClient)ari.AsyncState;
    try {
        tcpi.EndConnect(ari);
    } catch { }
    if (tcpi.Connected) {
        return; //return IAsyncResult and waitone will be true
    }
    //otherwise it will close the tcpi and never return, causing the timeout to kickin.
    tcpi.Close();
}, tcp);
#endregion
#region If timed out, or not connected return null
if (!ar.AsyncWaitHandle.WaitOne(_connectTimeout, false) || !tcp.Connected) {
    return null; //this is my use case, you might want to do something different
}
#endregion
return tcp;

Credit for linking another similar question:
@JeroenMostert

Credit for original solution on the similar question:
@Adster

Am I closing the TCP Client efficiently?

1 Answers1