0

I have a fairly general c# while loop question.

This code should continue to execute only after the RDP session has truly disconnected.

When the Connected property is changed to 0 it means that the RDP session connection has truly terminated. When the property is 1 it is still connected and the connection has not yet terminated.

Does anyone see anything inherently bad about this code? Is there a better way to go about it?

private void Reconnect()
{
    rdp1.Disconnect(); // force the RDP session to disconnect
    while (rdp1.Connected == 1) // true as long as RDP is still connected
    {
        // do nothing
    } 
    rdp1.Connect(); // execute this code after while loop is broken
}

/**************************************************************/

Here's the final code I used per James' answer. The counter suffices as the timeout for my purpose.

            int i = 0;
            rdp1.Disconnect();
            while (rdp1.Connected == 1)
            {
                if (i == 1000 * 10) break;
                else Thread.Sleep(100);
                i++;
            }
            rdp1.Connect();
IanK.CO
  • 563
  • 5
  • 13
  • 1
    Is there anything at all that could cause the boolean test to fail? Maybe the server is busy and never gets around to executing the disconnect or the command gets lost somewhere in a data corruption... Users might need a way to break out of it. Idk. – D. Ben Knoble Mar 17 '15 at 01:40
  • Yes, I think that is my worry, that if the rdp1 object were to fail for some reason and the state change was not detected, the user would need a break of some sort. Maybe just a timeout or a try/catch could work. – IanK.CO Mar 17 '15 at 01:42
  • James' answer has good info; I still think you need a way for a user to say whoa something happened get me out. – D. Ben Knoble Mar 17 '15 at 01:44
  • @Ben Knoble: yes, it's always good to have an exit. You never know what bugs might cause the code to get stuck and never disconnect. In that case, a simple while loop would go into an infinite loop. Throwing an exception after a given amount of time is the proper way to handle that. I'll update my answer accordingly. – James Mar 17 '15 at 12:22

2 Answers2

1

You should do something in the body of loop, or it will consume all your CPU (at least for one core). Usually in this type of loop, you'd sleep for a while using System.Threading.Thread.Sleep(100) or something. Sleep takes the number of milliseconds to wait before checking the while condition again. Ideally, the RDP object would have a mutex or event or something you could just block on until it was disconnected, but it wouldn't surprise me if they left that out.

EDIT: As Ben pointed out, it's always a good idea to have a way out of the loop as well. Something like this (your stated answer will depend on the CPU speed, which could break in the future when CPUs are much faster):

DateTime stop = DateTime.UtcNow.AddSeconds(30);
while (rdp1.Connected)
{
    if (DateTime.UtcNow > stop) throw new ApplicationException ("RDP disconnect timeout!");
    System.Threading.Thread.Sleep (100);
}

Of course you will probably want to specify the timeout with a constant, a readonly TimeSpan, or a dynamically configurable TimeSpan rather than a magic number, and you should probably have a specific exception class for this case.

James
  • 3,551
  • 1
  • 28
  • 38
  • 1
    (Sleeping or yielding for *any* time - eg. 0 ms, which is almost like 'yield' - will generally sufficiently non-toaster a CPU.) – user2864740 Mar 17 '15 at 01:43
  • Cool, I did have a Thread.Sleep(100); in there originally. I'll add that back in and at least have some way of getting the user out. – IanK.CO Mar 17 '15 at 01:46
  • 1
    @user2864740: Yes, technically that's correct in the simple case. However, in practice you often don't know how expensive the loop test is (if it's getting the value of a property--it could do _anything_), and if you have lots of loops like this, as well as other programs competing for CPU, waiting for more than a millisecond is probably wise. You rarely need the loop to break so quickly anyway. In fact, sometimes you will need to sleep a bit _after_ the loop exits because things aren't always really finished when they say they are (`File.Delete` for example). – James Mar 17 '15 at 01:50
  • Even sleeping, you'll still freeze your UI if this code is running on the UI thread. – Blorgbeard Mar 17 '15 at 03:27
  • @Blorgbeard: As with any I/O operation, yes it would. In those situations, the same code still applies, it should just be on a separate thread from the UI thread. I suspect there are many questions and answers about that issue. Factoring that into the block of code given by OP may or may not address that issue. It's impossible to say without more context. – James Mar 17 '15 at 12:37
1

Set a timeout for the purpose

private void Reconnect()
{
    timeOut = false;
    new System.Threading.Thread(new System.Threading.ThreadStart(setTimeout)).Start();
    rdp1.Disconnect(); 
    while (rdp1.Connected == 1 && !timeOut);
    rdp1.Connect();
}

bool timeOut = false;

void setTimeout()
{
    System.Threading.Thread.Sleep(7000);
    timeOut = true;
}
Abdul Saleem
  • 10,098
  • 5
  • 45
  • 45
  • This will consume all the CPU on the main thread for up to 7 seconds and then will try to connect even if the object wasn't disconnected (ie. If seven seconds elapsed and the connection was still active). That doesn't sound like a good idea. – James Mar 17 '15 at 03:08
  • He is connecting on some other thread, and want to wait on the main thread itself. Now turning on a flag on another thread. So whats the problem? – Abdul Saleem Mar 17 '15 at 04:13
  • I think the question was more centered around if the while loop was correct, it felt dangerous to not do anything inside the loop. I wasn't necessarily looking for the best performing solution, just that the logic is sound with the while loop. I've added a quick counter to time the issue which will suffice for now. – IanK.CO Mar 17 '15 at 04:59
  • @Sakya: regardless of what thread the actual connection happens on (that is not clear from this code), your code will consume all CPU on the main thread for no reason. On a single-core system, this could actually prevent the operation from happening because the CPU is so busy spinning in the while loop checking for the disconnection that it might not give any time to other threads. What is the point of starting up another thread just to set a flag after seven seconds? You can just keep track of the current time and stop spinning after seven seconds. Starting up a new thread is VERY expensive. – James Mar 17 '15 at 12:18
  • Thanks. I got that. But still helps in some situation, if we want the user do nothing in the UI the mean time.. – Abdul Saleem Mar 17 '15 at 12:22
  • Look at the updated answer, there the Thread sleeps for 10 * 100 * 1000 milliseconds – Abdul Saleem Mar 17 '15 at 12:26