3

I have a program that needs to scan for other devices running my program on the network. The solution I came up with was to call each ipAddress to see if my program is running.

The code below is completely blocking the cpu:-

using Newtonsoft.Json;
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Text;
using System.Threading;
using System.Threading.Tasks;

namespace FileWire
{
    class SearchNearby
    {

        private bool pc_search_cancelled = false;
        private SynchronizedCollection<Thread> PCSearchThreadList;
        private ConcurrentDictionary<String, String> NearbyPCList;

        public void NewPcFound(string s, string s1)
        {
            Console.WriteLine(string.Format("PC Found at: {0}    PC Name: {1}", s, s1));
        }

        public SearchNearby()
        {
            startPCScan();

            while (true)
            {
                bool isAnyAlive = false;
                foreach(Thread t in PCSearchThreadList)
                {
                    isAnyAlive |= t.IsAlive;
                }
                if (!isAnyAlive)
                {
                    Console.WriteLine("Search Complete");
                    foreach (var a in NearbyPCList)
                    {
                        Console.WriteLine(a.Key + " ;; " + a.Value);
                    }
                    startPCScan();
                }
                Thread.Sleep(100);
            }
        }


        private void startPCScan()
        {
            PCSearchThreadList = new SynchronizedCollection<Thread>();
            NearbyPCList = new ConcurrentDictionary<String, String>();
            pc_search_cancelled = false;
            String add = "";
            System.Net.IPAddress[] ad = System.Net.Dns.GetHostByName(System.Net.Dns.GetHostName()).AddressList;
            foreach (System.Net.IPAddress ip in ad)
            {
                add += ip.ToString() + "\n";
            }
            bool connected;
            if (add.Trim(' ').Length == 0)
            {
                connected = false;
            }
            else
            {
                connected = true;
            }
            if (connected)
            {
                try
                {
                    String[] addresses = add.Split('\n');
                    foreach (String address in addresses)
                    {
                        int myIP = int.Parse(address.Substring(address.LastIndexOf(".") + 1));
                        for (int def = 0; def <= 10; def++)
                        {
                            int finalDef = def;

                            for (int j = 0; j < 10; j++)
                            {
                                string finalJ = j.ToString();
                                Thread thread = new Thread(new ThreadStart(() =>
                                {

                                    if (!pc_search_cancelled)
                                    {
                                        for (int i = (finalDef * 25); i < (finalDef * 25) + 25 && i <= 255; i++)
                                        {
                                            if (!pc_search_cancelled)
                                            {
                                                if (i != myIP)
                                                {
                                                    String callToAddress = "http://" + address.Substring(0, address.LastIndexOf(".")) + "." + i + ":" + (1234 + int.Parse(finalJ)).ToString();
                                                    String name = canGetNameAndAvatar(callToAddress);
                                                    if (name != null)
                                                    {
                                                        NearbyPCList[callToAddress] = name;
                                                        NewPcFound(callToAddress, name);
                                                    }
                                                }
                                            }
                                        }
                                    }

                                }));
                                PCSearchThreadList.Add(thread);
                                thread.Start();
                            }
                        }
                    }
                } catch (Exception e) {
            }
        }
    }
        private String canGetNameAndAvatar(String connection)
        {
            String link = connection + "/getAvatarAndName";
            link = link.Replace(" ", "%20");
            try
            {

                var client = new HttpClient();
                client.Timeout = TimeSpan.FromMilliseconds(500);
                var a = new Task<HttpResponseMessage>[1];
                a[0] = client.GetAsync(link);
                Task.WaitAll(a);
                var b = a[0].Result.Content.ReadAsStringAsync();
                Task.WaitAll(b);
                
                Console.WriteLine(b.Result);
                
                string result = b.Result;
                result = result.Substring(result.IndexOf("<body>") + 6, result.IndexOf("</body>") - (result.IndexOf("<body>") + 6));
                AvtarAndName json = JsonConvert.DeserializeObject<AvtarAndName>(result);
                if (json != null)
                {
                    return json.name;
                }
            }
            catch
            {
                return null;
            }
            return null;
        }
    }

}

This is the exact C# version of the java code I was using in Java:-

import com.sun.istack.internal.Nullable;
import org.apache.http.*;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.impl.client.DefaultHttpClient;
import org.apache.http.params.CoreConnectionPNames;
import org.apache.http.params.HttpParams;
import org.json.JSONObject;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.net.InetAddress;
import java.net.NetworkInterface;
import java.net.SocketException;
import java.net.URI;
import java.util.*;
import java.util.concurrent.CopyOnWriteArrayList;

public class PCScan {

    private static boolean pc_search_cancelled = false;
    private static List<Thread> PCSearchThreadList;
    private static HashMapWithListener<String, String> NearbyPCList;
    public static void main(String[] args) {

        start();

        while (true) {
            int numCompleted = 0;
            for (Thread t : PCSearchThreadList) {
                if (!t.isAlive()) {
                    numCompleted++;
                }
            }
            if (numCompleted == PCSearchThreadList.size()) {
                start();
            }
        }
    }

    private static void start() {

        try {
            startPCScan();
        } catch (SocketException e) {
            e.printStackTrace();
        }

        NearbyPCList.setPutListener(new HashMapWithListener.putListener() {
            @Override
            public void onPut(Object key, Object value) {
                System.out.println(key.toString() + ";;" + value.toString());
            }
        });
    }

    private static void startPCScan() throws SocketException {
        pc_search_cancelled = false;
        PCSearchThreadList = new CopyOnWriteArrayList<>();
        NearbyPCList = new HashMapWithListener<>();
        Enumeration<NetworkInterface> enumeration = NetworkInterface.getNetworkInterfaces();
        boolean connected;
        String add = "";
        while (enumeration.hasMoreElements()) {
            NetworkInterface interfacea = enumeration.nextElement();
            if (!interfacea.isLoopback()) {
                Enumeration<InetAddress> enumeration1 = interfacea.getInetAddresses();

                while (enumeration1.hasMoreElements()) {
                    String address = enumeration1.nextElement().getHostAddress();
                    if (address.split("\\.").length == 4) {
                        add += address + "\n";
                    }
                }
            }

        }

        System.out.println(add);
        connected = true;
        if (connected) {
            try {
                String[] addresses = add.split("\n");
                addresses = new HashSet<String>(Arrays.asList(addresses)).toArray(new String[0]);
                for (String address : addresses) {
                    int myIP = Integer.parseInt(address.substring(address.lastIndexOf(".") + 1));
                    for (int def = 0; def <= 10; def++) {
                        int finalDef = def;

                        for (int j = 0; j < 10; j++) {
                            int finalJ = j;
                            Thread thread = new Thread(new Runnable() {
                                @Override
                                public void run() {
                                    if (!pc_search_cancelled) {
                                        for (int i = (finalDef * 25); i < (finalDef * 25) + 25 && i <= 255; i++) {
                                            if (!pc_search_cancelled) {
                                                if (i != myIP) {
                                                    String callToAddress = "http://" + address.substring(0, address.lastIndexOf(".")) + "." + i + ":" + String.valueOf(Integer.parseInt("1234") + finalJ);
                                                    String name = canGetNameAndAvatar(callToAddress);
                                                    if (name != null) {
                                                        NearbyPCList.put(callToAddress, name);
                                                    }
                                                }
                                            }
                                        }
                                    }
                                }
                            });
                            PCSearchThreadList.add(thread);
                            thread.start();
                        }
                    }
//                        }
//                    }).start();
                }
            } catch (Exception e) {
            }
        }
    }
    private static String canGetNameAndAvatar(String connection) {
        String link = connection + "/getAvatarAndName";
        link = link.replaceAll(" ", "%20");
        try {
            HttpClient client = new DefaultHttpClient();
            HttpParams httpParams = client.getParams();
            httpParams.setParameter(
                    CoreConnectionPNames.CONNECTION_TIMEOUT, 500);
            HttpGet request = new HttpGet();
            request.setURI(new URI(link));
            HttpResponse response = client.execute(request);
            BufferedReader in = new BufferedReader(new
                    InputStreamReader(response.getEntity().getContent()));

            StringBuffer sb = new StringBuffer("");
            String line="";

            while ((line = in.readLine()) != null) {
                sb.append(line);
                break;
            }
            in.close();
            String result = sb.toString();
            result = result.substring(result.indexOf("<body>") + 6, result.indexOf("</body>"));
            JSONObject json = new JSONObject(result);
            if (json != null) {
                return json.getString("name");
            }
        }
        catch (Exception ignored){
            return null;
        }
        return null;
    }
    static class HashMapWithListener<K, V> extends HashMap<K, V> {

        private putListener PutListener;
        public void setPutListener(putListener PutListener) {
            this.PutListener = PutListener;
        }

        @Nullable
        @Override
        public V put(K key, V value) {
            PutListener.onPut(key, value);
            return super.put(key, value);
        }

        interface putListener {
            public void onPut(Object key, Object value);
        }

    }

}

The java code runs absolutely fine and only uses about 20 percent cpu while c# code absolutely locks the PC. I tried Webclient, webrequest, httpClient. All have literally the same performance.

I need the code to be in c# as I can't include whole JRE in my program since it is too large. The rest of my program and GUI is in WPF format.

Also, I need the code to take a maximum of 50seconds while scanning ports 1234-1243. This code also works absolutely fine even on a midrange android phone. So, I don't know what the problem is.

Mark Rotteveel
  • 100,966
  • 191
  • 140
  • 197
saksham
  • 81
  • 1
  • 7
  • 2
    Read [You're using HttpClient wrong and it is destabilizing your software](https://www.aspnetmonsters.com/2016/08/2016-08-27-httpclientwrong/) and the follow-up [You're (probably still) using HttpClient wrong and it is destabilizing your software](https://josef.codes/you-are-probably-still-using-httpclient-wrong-and-it-is-destabilizing-your-software/). Also, use async/await fully in your program so that you're not just running threads waiting for web requests to complete. – ProgrammingLlama Jun 27 '21 at 08:36
  • Thank you for the links. But the main concern is that it does work with either webclient or webrequest either. Are they also internally not efficient at opening multiple connections? – saksham Jun 27 '21 at 08:45
  • "but the main concern" - so you don't care about exhausting all of the available sockets on the machine, thus leading to failures? Okay... Anyway, as I already said: you should use async/await fully. Async/await actually suspends the thread and suspends the work while IO work is occurring (you know, like a web request), and then resumes when it comes back. Instead you're using the async/await-capable `HttpClient` but throwing away this functionality, instead creating a thread for every single request, meaning that you have the CPU spinning waiting for the request to complete. – ProgrammingLlama Jun 27 '21 at 08:47
  • They are internally efficient, but you are writing code that is inefficient. – ProgrammingLlama Jun 27 '21 at 08:47
  • Just to be sure (I am new to c#). I want to ask that, does using task.wait() actively waste CPU cycles? – saksham Jun 27 '21 at 08:58
  • 1
    For your use case, probably multicast broadcasts might be a better fit. Search them up. – Mat J Jun 27 '21 at 09:43
  • @MatJ You are right. I didn't even know such a thing existed and was wondering why other application were able to find devices so quickly. Thanks man. Also, I could not solve the problem of c# even after awaits or taking up too much time. Still I will be using them as fall back since UDP does not work on mobile hotspot and some Routers – saksham Jun 27 '21 at 14:58

2 Answers2

-1

I would suggest something like this (I've simplified it for the sake of an example):

private static HttpClient _client = new HttpClient() { Timeout = TimeSpan.FromMilliseconds(500) };

private async Task<Something> GetSomething(string url)
{
    using (HttpResponseMessage response = await _client.GetAsync(url))
    {
        string json = await response.ReadAsStringAsync();
        return JsonConvert.DeserializeObject<Something>(json);
    }
}

private async Task<Something[]> GetSomethings(string[] urls)
{
    IEnumerable<Task<Something>> requestTasks = urls.Select(u => GetSomething(u));
    Something[] results = await Task.WhenAll<Something>(requestTasks);
    return results;
}

You should also make the method calling GetSomethings async, and await it, and do the same all the way up the call chain.

async/await uses a thread pool to execute, and the thread is actually suspended while the IO part of the request occurs, meaning that no CPU time is used during this period. When then IO part is done, it resumes the code at the await.

Related information:

ProgrammingLlama
  • 36,677
  • 7
  • 67
  • 86
-2

You are using threads and multithreading completely wrong.

Because I cannot really understand what you are trying to do because everything is cramped into one function, I cannot provide you with a more detailed solution.

But let me suggest the following: I understand, you want to execute some operation in the background connecting to some other computer, try something like this

var taskList = new List<Task>();
foreach (var pc in computers)
{
  var currentPcTask = Task.Run(() => DoYourWorkForSomePcHere(pc));
  taskList.Add(currentPcTask);
}
Task.WaitAll(taskList.ToArray());

This will be very CPU efficient.

phimath
  • 141
  • 8
  • 2
    `Task.Run(() => WebRequest())` is aboslutely not the correct way to do this. It will eat up your thread pool while you wait for requests to complete. – ProgrammingLlama Jun 27 '21 at 08:48
  • @Llama how is async waiting eating up the thread pool? Prove me wrong. – phimath Jun 27 '21 at 09:35
  • I didn't say `async` is eating up the thread pool. – ProgrammingLlama Jun 27 '21 at 09:36
  • So then the only difference between your answer and mine is, that you use an async method with a return value, while I assumed that he uses synchronous code without a return value. And you give me a downvote for that? – phimath Jun 27 '21 at 09:38
  • 1
    No, that's not "the only difference". `Task.WaitAll` that OP is using will block the thread it runs in until the task completes. This is not the async way. By using `Task.Run` to (albeit indirectly) execute this code, you are forcing this to run in a thread on the thread pool ([docs](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.run?view=net-5.0)). Therefore, writing it like this, we are blocking a thread pool thread until the request completes. – ProgrammingLlama Jun 27 '21 at 09:46
  • My answer uses async/await all the way, so we won't actually be using any CPU while the request is executing. This is because the execution is suspended until the request completes (as far as I'm aware, it's basically a state machine under the hood). This also means that the calling method, awaiting the web request task, is suspended while there is no CPU-bound work to do. Therefore, this is only occupying a thread pool thread while work actually needs to be done (not while we're waiting for the request to complete). – ProgrammingLlama Jun 27 '21 at 09:46
  • See [this answer](https://stackoverflow.com/a/33764670/3181933) for some more information. It's not exactly the same, but as we covered OP is using `Task.WaitAll` which is synchronously waiting for the request to complete, so it's analogous to the linked question. – ProgrammingLlama Jun 27 '21 at 09:51
  • The biggest performance problem in the original code comes from actively checking if a thread (request operation) completed in a while loop, not from the Web Request. I removed this busy waiting with my suggestion. Indeed, async uses a state machine. This way, the Task.WaitAll still performs better than the original code, although it might consume more resources than your all-async approach, because - to my knowledge - it uses a spin lock and notification to wait for the other tasks. – phimath Jun 27 '21 at 09:52
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/234249/discussion-between-phimath-and-llama). – phimath Jun 27 '21 at 09:52