2
\$\begingroup\$

I'm creating my own little toolbox for pinging IP interfaces (for the test, only in localhost). In the future I'm thinking of piping the output to a text file. But I'd like to focus on improving the performance for now.

For now, scanning a class A IP range is pretty fast, but for a B range (65025 addresses) it's very long. Taking in excess of 5 minutes. Here is my code:

#!/usr/bin/python3

import threading
import os


# Define a function for the thread
def pingBox( threadName, host):
    response = os.system("ping -c 1 "+host) 
    print('response for pinging %s is %s' %(threadName, response))
    if response == 0:
        print ('%s : is up' %host)
    else:
        print ('%s : is down' %host)
# Create one thread by ping
def createThread(host):
    try:
        tlist = []
        tlist.append(
            threading.Thread(
                target=pingBox,
                kwargs={
                    "threadName": "Thread:{0}".format(host),
                    "host":host
                }
        ))

        for t in tlist:
            t.start()

        for t in tlist:
            t.join()
    except Exception as e:
        print(e)

def rangeSelector(ipRange):
    if ipRange== 'A' or ipRange== 'a':
        for i in range (0,255):
            createThread('127.0.0.'+str(i))
    elif ipRange== 'B' or ipRange== 'b':
        for i in range (0,255):
            for j in range(0,255):
                createThread('127.0.'+str(i)+'.'+str(j))
    else:
        pass

if __name__ == "__main__":
    ipRange=input('Choose range A|B :')
    rangeSelector(ipRange)
\$\endgroup\$
5
  • \$\begingroup\$ You do know that address range 127.0.0.0/24 is assigned to localhost addresses (which respond very fast to ping on normal machines), but the rest of 127.0.0.0/16 is not (and likely won't respond at all), don't you? \$\endgroup\$ Commented Dec 17, 2019 at 16:12
  • \$\begingroup\$ @Warok, honestly, I've found the optimized solution since yesterday. But looking at this situation that fell into "disrepute", I feel doubt that such an effort would be adequately assessed. Too much of "bad energy" here ... \$\endgroup\$ Commented Dec 18, 2019 at 12:41
  • \$\begingroup\$ @RomanPerekhrest, could you anyway post it please? I just wanted some advices, how to create my threads faster, or to speed up my script. \$\endgroup\$
    – Warok
    Commented Dec 18, 2019 at 13:03
  • \$\begingroup\$ @RomanPerekhrest If your solution differs much from mine, then I too would like to see your insight. \$\endgroup\$
    – Peilonrayz
    Commented Dec 18, 2019 at 16:02
  • \$\begingroup\$ @Peilonrayz, Yes, I'll put my contribution soon (writing it now). Thanks for participation. \$\endgroup\$ Commented Dec 18, 2019 at 16:10

2 Answers 2

4
\$\begingroup\$

Towards better design, functionality and performance

Start with good names: that means following Python naming conventions and give a meaningful names to your identifiers/functions/classes:

rangeSelector ---> ping_network_range
createThread ---> start_threads
pingBox ---> check_host

As @Peilonrayz already mentioned range(256) is the valid range for your case.

The former createThread function tried to create a single thread with start ing and join ing it at one step. But that undermines the benefit of using threading for parallelizing computations. The crucial mechanics is that all treads are initiated/started at once, then, we join them (awaiting for their finishing) at next separate phase.
On constructing a threading.Thread no need to pass custom thread name through kwargs={"threadName": ...} - the Thread constructor already accepts name argument for the thread name which is then accessed within target function via threading.current_thread().name.

The former pingBox function used os.system function (executes the command (a string) in a subshell) which is definitely not a good choice.
The more robust, powerful and flexible choice is subprocess module:

The subprocess module allows you to spawn new processes, connect to their input/output/error pipes, and obtain their return codes. This module intends to replace several older modules and functions: os.system, os.spawn*
...
The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle. For more advanced use cases, the underlying Popen interface can be used directly.

Some good, user-friendly description - on this SO link.

Furthermore, the ping command (OS network tool) can be speed up itself through adjusting specific options. The most significant ones in such context are:

  • -n (No attempt will be made to lookup symbolic names for host addresses. Allows to disable DNS lookup to speed up queries)
  • -W <number> (Time to wait for a response, in seconds. The option affects only timeout in absence of any responses, otherwise ping waits for two RTTs)
  • -i interval (Wait interval seconds between sending each packet. The default is to wait for one second between each packet normally, or not to wait in flood mode. Only super-user may set interval to values less than 0.2 seconds)

The difference would be more noticeable on sending more than one packet (-c option).
In your case I'd apply -n and -q options. With -q option allows to quite the output since we'll get the returned code which itself indicates whether a particular host is reachable across an IP network. subprocess.run allows to access the returncode explicitly.


The 2 nested for loops within the former rangeSelector function is flexibly replaced with itertools.product routine to compose an efficient generator expression yielding formatted IP addresses:

from itertools import product
...
start_threads(f'127.0.{i}.{j}' for i, j in product(range(256), range(256)))

Designing command-line interface

As your program is planned to be used as part of pipeline, instead of hanging on interactive, blocking call of input('Choose range A|B :') - the more flexible and powerful way is using argparse module that allows building an extended and flexible command-line interfaces with variety of options of different types and actions.
For ex. the allowed network classes names can be supplied through choices:

...
parser = ArgumentParser(description='Ping network addresses by network class')
parser.add_argument('-c', '--nclass', choices=('A', 'B'), required=True, help='Choose class A or B')

The final optimized implementation:

import threading
from argparse import ArgumentParser
from itertools import product
import subprocess


def check_host(host: str):
    return_code = subprocess.run(["ping", "-c", "1", "-n", "-q", host],
                                 stdout=subprocess.DEVNULL,
                                 stderr=subprocess.DEVNULL).returncode
    print(f'response for pinging {threading.current_thread().name} is {return_code}')

    status = 'up' if return_code == 0 else 'down'
    print(f'{host} : is {status}')


def start_threads(addr_range):
    for addr in addr_range:
        t = threading.Thread(target=check_host, args=(addr,), 
                             name=f'Thread:{addr}')
        t.start()
        yield t


def ping_network_range(net_class: str):
    net_class = net_class.upper()
    if net_class == 'A':
        threads = list(start_threads(f'127.0.0.{i}' for i in range(256)))
    elif net_class == 'B':
        threads = list(start_threads(f'127.0.{i}.{j}' 
                                     for i, j in product(range(256), range(256))))
    else:
        raise ValueError(f'Wrong network class name {net_class}')

    for t in threads:
        t.join()


if __name__ == "__main__":
    parser = ArgumentParser(description='Ping network addresses by network class')
    parser.add_argument('-c', '--nclass', choices=('A', 'B'), 
                        required=True, help='Choose class A or B')
    args = parser.parse_args()
    ping_network_range(args.nclass)

Sample usage (with timing, under Unix time command):

time python3 ping_network_range.py -c B > test.txt

real    2m4,165s
user    2m17,660s
sys     4m35,790s

Sample tail contents of resulting test.txt file:

$ tail test.txt
response for pinging Thread:127.0.255.250 is 0
127.0.255.250 : is up
response for pinging Thread:127.0.255.252 is 0
127.0.255.252 : is up
response for pinging Thread:127.0.255.253 is 0
127.0.255.253 : is up
response for pinging Thread:127.0.255.255 is 0
127.0.255.255 : is up
response for pinging Thread:127.0.255.254 is 0
127.0.255.254 : is up
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Nice answer, contains a lot that mine doesn't. \$\endgroup\$
    – Peilonrayz
    Commented Dec 18, 2019 at 18:25
  • 1
    \$\begingroup\$ Wow, thanks a lot for the help :) \$\endgroup\$
    – Warok
    Commented Dec 19, 2019 at 10:23
2
\$\begingroup\$
  • You should run a linter on your program, you have quite a few PEP 8 issues. A lot are from non-standard whitespace usage, and using cammelCase rather than snake_case. Conforming to one standard - PEP 8 - allows others to more easily address your code. This is as all code looks the same and so can be easily read.
  • At one time % formatting was deprecated in the Python docs. Given that f-strings are available in Python now, I would recommend converting to either str.format or f-strings to increase readability with modern Python best practices.
  • You can change pingBox to use a ternary to DRY your code.
  • You should be able to see that in createThread tlist is only going to have 1 item. On creation you're going to start the thread and then wait for it to finish running.

    The problem is that you're Thread.joining before you've started all the other threads. This is simple to fix, you just need to build the threads before joining them.

  • You can use str.lower to simplify your rangeSelector if statements.

  • If your range starts at 0, then you don't need to specify 0.
  • I think you have a bug, 255 is a valid address. You currently are ignoring it tho. You need to specify 256 in the range.
  • You can use a generator comprehension to make all the IPs you want, that you need to pass to create_threads.
#!/usr/bin/python3

import threading
import os


def ping_box(thread_name, host):
    response = os.system("ping -c 1 " + host)
    print(f'Response for pinging {thread_name} is {response}')
    status = 'down' if response else 'up'
    print(f'{host}: is {stetus}')


def create_threads(ips):
    for ip in ips:
        thread = threading.Thread(
            target=ping_box,
            kwargs={
                "thread_name": f"Thread:{ip}",
                "host": ip
            }
        )
        thread.start()
        yield thread


def range_selector(ip_range):
    if ip_range.lower() == 'a':
        ips = (
            f'127.0.0.{i}'
            for i in range(256)
        )
    elif ip_range.lower() == 'b':
        ips = (
            f'127.0.{i}.{j}'
            for i in range(256)
            for j in range(256)
        )
    else:
        ips = ()
    for thread in list(create_threads(ips)):
        thread.join()


if __name__ == "__main__":
    range_selector(input('Choose range A|B :'))

Please note the list around create_threads. This is to avoid the laziness of generator functions, which is not something we want.

\$\endgroup\$
4
  • \$\begingroup\$ I'm trying to understand your code, but it seems there is an error : Exception in thread Thread-2361: Traceback (most recent call last): File "/usr/lib/python3.6/threading.py", line 916, in _bootstrap_inner self.run() File "/usr/lib/python3.6/threading.py", line 864, in run self._target(*self._args, **self._kwargs) TypeError: ping_box() got an unexpected keyword argument 'threadName' \$\endgroup\$
    – Warok
    Commented Dec 18, 2019 at 15:15
  • \$\begingroup\$ @Warok Oh yeah there's an error there. You should be able to figure it out. Note, threadName vs. thread_name. \$\endgroup\$
    – Peilonrayz
    Commented Dec 18, 2019 at 15:40
  • \$\begingroup\$ I knew you did it on purpose ;) Thanks for the help and the improvements \$\endgroup\$
    – Warok
    Commented Dec 18, 2019 at 15:42
  • \$\begingroup\$ @Warok Nope, just a small typo. :( No problem, :) \$\endgroup\$
    – Peilonrayz
    Commented Dec 18, 2019 at 16:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.