2

Im creating a remote administration tool for my university degree. Im currently really stuck on a bug in my code and was wondering if anyone could shed some light on my issue.

I have Applications, a server and a client. The server runs fine. However the client is the application that freezes.

Before connecting to the server the client works perfectly. When connected to the server the client is constantly frozen on the screen.

I've narrowed the bug down to a specific piece of code, without this code running the application doesn't freeze. However the application also doesn't work.

Here is an example of that code:

private static void ReceiveResponse()
    {

        var buffer = new byte[2048]; //The receive buffer

        try
        {
            int received = 0;
            if (!IsLinuxServer) received = _clientSocket.Receive(buffer, SocketFlags.None); //Receive data from the server
            else received = _sslClient.Read(buffer, 0, 2048);
            if (received == 0) return; //If failed to received data return
            var data = new byte[received]; //Create a new buffer with the exact data size
            Array.Copy(buffer, data, received); //Copy from the receive to the exact size buffer

            if (isFileDownload) //File download is in progress
            {
                Buffer.BlockCopy(data, 0, recvFile, writeSize, data.Length); //Copy the file data to memory

                writeSize += data.Length; //Increment the received file size

                if (writeSize == fup_size) //prev. recvFile.Length == fup_size
                {


                    using (FileStream fs = File.Create(fup_location))
                    {
                        Byte[] info = recvFile;
                        // Add some information to the file.
                        fs.Write(info, 0, info.Length);
                    }

                    Array.Clear(recvFile, 0, recvFile.Length);
                    SendCommand("frecv");
                    writeSize = 0;
                    isFileDownload = false;
                    return;
                }
            }

            if (!isFileDownload) //Not downloading files
            {
                string text = (!IsLinuxServer) ? Encoding.Unicode.GetString(data) : Encoding.UTF8.GetString(data); //Convert the data to unicode string
                string[] commands = GetCommands(text); //Get command of the message



                foreach (string cmd in commands) //Loop through the commands
                {
                    HandleCommand(Decrypt(cmd)); //Decrypt and execute the command
                }
            }
        }
        catch (Exception ex) //Somethind went wrong
        {
            MessageBox.Show(ex.Message);
            RDesktop.isShutdown = true; //Stop streaming remote desktop
           // MessageBox.Show("Connection ended");
        }
    }

This code is how the user receives a request from the server. So it is in a timer to be run every 100ms.

Im wondering if the timer has anything to do with it. Before it was in a While(true) loop and I had the same issue which makes me think that it's the actual code making the UI freeze.

The code still works even if the application is frozen, everything on the app works, apart from the UI.

This is really frustrating and I can't really see anything wrong with the code that would cause the application to freeze.

Any help would be much appreciated.

Thankyou In advance.

Jeremy Griffin
  • 107
  • 1
  • 8
  • You need to run this code in an `asynchronous` way. – Paul Karam Apr 16 '18 at 14:05
  • 1
    Anything you do on the UI thread prevents the UI thread doing anything else. That's why you aren't supposed to do anything that runs for a long time on the UI thread. If you need to gather data then do that on a secondary thread and then only marshal back to UI thread to actually update the UI. Do some reading on parallelism, multi-threading and asynchronous programming. – jmcilhinney Apr 16 '18 at 14:06
  • Does it seem strange to you that you wrote `if (x) { ... } if (!x) { ... }` and not `if (x) { ... } else { ... } ` ? It seems strange to me. Why did you choose this way of writing an if-else? – Eric Lippert Apr 16 '18 at 16:58
  • More generally, it seems to me like this code is doing four things: (1) file download over a normal socket, (2) file download over an ssl socket, (3) text download over a normal socket, (4) text download over an ssl socket. There seems to be some abstractions missing here that could be built easily out of existing parts, that would make this code much simpler and easier to understand. **Computer programming is the art of making useful abstractions**. Your code is suffering from a lack of abstraction here. – Eric Lippert Apr 16 '18 at 17:01
  • Consider what happens is the request takes > 100ms. – zaph Apr 16 '18 at 17:21

6 Answers6

3

Six answers so far and no helpful, correct or actionable advice. Ignore them.

I've narrowed the bug down to a specific piece of code, without this code running the application doesn't freeze

Good, that's the first step in diagnosing the problem. Go further. Exactly which methods in here are the ones that have high latency? My guess would be Receive or Read but there are several possible candidates, and it might be more than one of them.

Once you have identified each high latency method, determine whether it is slow because it is using CPU or waiting for IO.

If it is slow because it is using CPU, what you want to do is move that operation on to another CPU and wait for the result. You can do that as the other answers suggest: use Task.Run to move the work onto a background thread, and then await the result.

If it is slow because it is waiting for IO then do not move the operation to a background thread unless you have no other choice. The background worker APIs are designed to offload work to other CPUs, and IO is not CPU-bound work. What you want to do here is use the async IO operations. If the blocker is Receive, say, then be using ReceiveAsync instead and await the resulting Task.

You should also make your method async and return a Task which is then awaited by its caller, and so on up to the event handler that kicks the whole thing off.

Keep doing this process until you have identified every operation in your method that takes more than 30 milliseconds, and made it either async on a worker thread, if CPU bound, or used an async IO method if IO bound. Your UI should then never have a delay of more than 30 ms.

Speaking of event handlers...

This code is how the user receives a request from the server. So it is in a timer to be run every 100ms.

That sounds very, very wrong. First of all, "receives a request from the server"? Servers do requests on behalf of clients, not vice versa.

Second, this sounds like a wrong way to deal with the problem.

If you asyncify this code properly there should be no need of a timer to constantly poll the socket. You should simply asynchronously read, and either the read succeeds and calls you back, or it times out.

Also, there's no loop here. You say that you run this code every 100 ms, and you read a maximum of 2K here, so you're reading 20K per second maximum no matter how saturated your network connection is, right? Does that seems right to you? because it seems wrong to me.

Eric Lippert
  • 647,829
  • 179
  • 1,238
  • 2,067
  • like the "asyncify" :D – Rand Random Apr 16 '18 at 14:53
  • Im using an I7700k So I don't think it's the CPU. The application isn't slow, it is simply frozen and unusable, I run the server as the same time as the client and the server is running perfectly. The client is on a timer to check to see if a request has been received back from the server. So the server might ask what is your computer name. The client will receive that request and send the data back to the server. – Jeremy Griffin Apr 16 '18 at 14:56
  • I think I understand what you're getting at however. Instead of trying to constantly loop until a response is received have some method in place so the Client knows when its had a request sent to it. – Jeremy Griffin Apr 16 '18 at 14:57
  • @JeremyGriffin: No, what I'm saying is you receive asynchronously and the receive calls you back when the data is there. – Eric Lippert Apr 16 '18 at 14:58
  • 1
    @JeremyGriffin: Don't *think*. *Know*. Use the tools at your disposal to analyze the program and find out where the UI thread is blocking for more than 30ms, and then categorize that as CPU or IO. **Work from a position of knowledge and understanding, not guessing and hoping**. – Eric Lippert Apr 16 '18 at 15:00
  • If I don't need a timer how is the code going to run again after the first response is read. If it awaits a response then surely after the first response it has nothing to wait for anymore and no reason to continue? – Jeremy Griffin Apr 16 '18 at 16:07
  • @JeremyGriffin: Then await a second response. – Eric Lippert Apr 16 '18 at 16:10
  • So I need to be able to hard code the number of responses my client is going to receive? Im extremely new to this Async method and I understand for you this might be so easy but it isn't for me. I've been looking at some tutorials online and im still unsure how im going to loop this to keep on trying to get a response? The ReceiveResponse() method doesn't return anything so im confused on that aspect also. – Jeremy Griffin Apr 16 '18 at 16:50
  • @JeremyGriffin: No, you don't hard code the number of responses. Right now you are using a timer to simulate an asynchronous loop by using the timer callback as a proxy for being notified of the state of the socket, and then *waiting* on the socket. **Write an actual asynchronous loop that is notified when the socket has data**. That's the benefit of asynchronous code: you want looping behaviour, you write a *loop*, not a timer with polling. – Eric Lippert Apr 16 '18 at 16:55
  • Eric is trying to give you the tools you need to go read a bit more information about asynchronous constructs @JeremyGriffin rather than re-write your code for you (which is fair enough). Basically, your socket operations are I/O bound. There are constructs (such as I/O completion ports) which allow the operating system to notify your code when data is available for reading (instead of you polling it). Your code then becomes a simple loop that `await`'s this I/O operation and importantly, _doesn't freeze your application_. Your application continues to happily run while waiting for data. – Simon Whitehead Apr 18 '18 at 00:20
  • The TLDR though, is basically: go read up what [`Stream.ReadAsync`](https://msdn.microsoft.com/en-us/library/hh137813(v=vs.110).aspx) does and how you might be able to use a network stream that uses these APIs. Wrapping a `Socket` in TPL-based code might be a bit advanced for you at this point.. so perhaps look at using another abstraction higher up the chain (`TcpListener` and a `TcpClient` using the `WriteAsync`/`ReadAsync` APIs, for example) – Simon Whitehead Apr 18 '18 at 00:29
1

you can make use of BackGroundWorker or make use of async/await both will resolve issue of freezing UI.

Code given below is for example you can google it , and use one of this can resolve your issue


As there is operation which is doing IO calls and in newer framework methods with **async is available suggestion to make use of async/awiat

async/await way (New way - .NET 4.5 for 4.0 version you will find nuget )

private async void Button_Click(object sender, RoutedEventArgs 
{
      var task = Task.Factory.StartNew( () => longrunnincode());
      var items = await task;
      //code after task get completed 
}

Back Ground worker (Old way but still supported and used in lot of old application as async/await was delivered in .NET 4.5 for 4.0 version you will find nuget)

private BackgroundWorker backgroundWorker1;
this.backgroundWorker1 = new BackgroundWorker(); 
this.backgroundWorker1.DoWork += new 
        DoWorkEventHandler(this.backgroundWorker1_DoWork);

private void backgroundWorker1_DoWork(object sender, DoWorkEventArgs e)
{
    BackgroundProcessLogicMethod();
}

private void backgroundWorker1_RunWorkerCompleted(object sender,

    RunWorkerCompletedEventArgs e)
{
  if (e.Error != null) MessageBox.Show(e.Error.Message);
    else MessageBox.Show(e.Result.ToString());
} 

private void StartButton_Click(object sender, EventArgs e)

{
    // Start BackgroundWorker
    backgroundWorker1.RunWorkerAsync(2000);
}

Don't use background workers for IO unless there is no other way. Make calls to the asynchronous IO APIs directly.

Pranay Rana
  • 175,020
  • 35
  • 237
  • 263
  • 1
    Don't use background workers for IO unless there is no other way. Make calls to the asynchronous IO APIs directly. – Eric Lippert Apr 16 '18 at 14:19
  • @EricLippert - thanks sir, i just added your comment in answer , a – Pranay Rana Apr 16 '18 at 14:23
  • Why are you doing exactly the thing you're saying shouldn't be done? – Servy Apr 16 '18 at 14:25
  • @Servy - apologies but not getting you .. I suggested both the ways as i used both , background worker i used in application when there was no async/await , once i got that i used in my new applications – Pranay Rana Apr 16 '18 at 14:26
  • You suggested two solutions that create background threads to do IO work, and zero solutions that actually do the IO work without creating/using background threads, which is the correct solution. Which syntax you use to create the background thread is just a personal preference, and doesn't affect the answer. – Servy Apr 16 '18 at 14:28
  • @Servy - but async/await approach is good one to use if new framework verison is there – Pranay Rana Apr 16 '18 at 14:29
  • @PranayRana It's not going to behave differently when used in this way. *If used correctly* it can make writing an asynchronous solution that *doesn't use any additional thread* easier to write, but this answer doesn't provide such a solution. – Servy Apr 16 '18 at 14:30
  • I'm using the most updated .Net Version available so using the async method should work for me. However is it the best method to use? – Jeremy Griffin Apr 16 '18 at 14:33
  • @JeremyGriffin - if you google it that is new updaed thing to stop freezing UI , that is the way i use in my application, – Pranay Rana Apr 16 '18 at 14:36
  • @PranayRana this receive response code needs to be run over and over again until a response is received, I do this with a winforms timer. Is this the wrong type of timer to use as apparently that also runs on the UI thread. Like have the var task and var await task in a timer? – Jeremy Griffin Apr 16 '18 at 14:43
  • @PranayRana Never touched this before so sorry if my questions seem silly. Im genuinely confused. – Jeremy Griffin Apr 16 '18 at 14:43
  • @JeremyGriffin - yes dont use UI timer , you should use System.Timer..chek here : http://www.albahari.com/threading/part3.aspx#_Timers i use this link for threeading stuff in my application its good in detail – Pranay Rana Apr 16 '18 at 14:49
0

So worked out a little work around method for it, didn't use async however I did just end up creating a different thread for this task.

The new thread let me used a while(true) loop to forever loop receive response.

private void btnConnect_Click(object sender, EventArgs e)
    {
        ConnectToServer();
        Thread timerThread = new Thread(StartTimer);
        timerThread.Start();
        //StartTimer();
        //timerResponse.Enabled = true;

    }
    private static void StartTimer()
    {
        while (true)
        {
            ReceiveResponse();
        }
    }

This worked good enough for what I needed to do.

Thanks all for the help!

Jeremy Griffin
  • 107
  • 1
  • 8
-1

It's more important in what context you are running this code. You are probably running it on the UI thread. You should defer heavy work to another worker thread or make the method async.

oviuan
  • 108
  • 5
-1

This is a common issue with GUI's. Events are supposed to finish ASAP. When one Event does not return, no other Event can run. Not even the code that Updates the UI with the last changes will be executed. And all Windows can tell the user is that the Programm is "not responding" (because it has not yet aknowledged the last Input Windows send it's way).

What you need to do is move the long running Operation into some form of Multitasking. There are many ways to do that, some involving multiple threads, some not. When starting with Multitasking in a GUI environment (wich is the ideal case), I would use the BackgroundWorker. He is not the thing you want to be seen using in productive code/later, but it is a good Beginners tool to learn the Idiosyncracies of Multitasking (Race Conditions, Exception Handling, Callbacks).

As Network operations are not usually known to be CPU bound, you may want to go for a Threadless way of Multitasking in the long run.

Christopher
  • 9,634
  • 2
  • 17
  • 31
-1

It sounds like it probably will be the timer. If you are using the Thread.Sleep() method, this will definitely be the issue. The use of this function locks the current thread, and since your function is not running asynchronously to your UI thread, the UI will also freeze.

To get around this problem, you can simply add the code you provided to an asynchronous Task, and use the Task.Delay() method to ensure the function will only execute every 100ms.

See this question for more information.

Harry
  • 197
  • 3
  • 16