8
\$\begingroup\$

This is only my third Python script. Be brutal with me. Any tips, tricks, best practices, or better usages would be great!

import socket
from concurrent.futures import ThreadPoolExecutor

THREADS = 512
CONNECTION_TIMEOUT = 1

def ping(host, port, results = None):
    try:
        socket.socket().connect((host, port))
        if results is not None:
            results.append(port)
        print(str(port) + " Open")
        return True
    except:
        return False

def scan_ports(host):
    available_ports = []
    socket.setdefaulttimeout(CONNECTION_TIMEOUT)
    with ThreadPoolExecutor(max_workers = THREADS) as executor:
        print("\nScanning ports on " + host + " ...")
        for port in range(1, 65535):
            executor.submit(ping, host, port, available_ports)
    print("\nDone.")
    available_ports.sort()
    print(str(len(available_ports)) + " ports available.")
    print(available_ports)

def main():
    scan_ports("127.0.0.1")

if __name__ == "__main__":
    main()
\$\endgroup\$
  • \$\begingroup\$ What is the purpose of this code? \$\endgroup\$ – Gareth Rees Jan 3 '14 at 0:17
  • \$\begingroup\$ @GarethRees Just learning from a book. If I were being malicious, I certainly wouldn't post the question under my real name. Haha \$\endgroup\$ – asteri Jan 3 '14 at 0:22
  • \$\begingroup\$ Follow up question here \$\endgroup\$ – Kuba Ober Sep 11 '15 at 18:51
11
\$\begingroup\$

It's not Pythonic to pass in a mutable object and use that to store results (like it is in C).

range(start, stop) is not inclusive of stop, so you have an off by one error as well.

Using a catch-all except line is also poor practice. It's important to catch expected exceptions and reraise the rest.

Also Python lists are thread safe only because of the GIL in CPython. This code would not be thread safe in other implementations such as Jython. Notice how your version always reports open ports in ascending order.

Threads are fine for IO bounded actions, but since pinging localhost is so fast GIL contention slows down performance in this case. Your implementation ran in 27 seconds on my laptop.

By comparison, my implementation ran in 2.76 seconds. Replacing map() with pool.map() led to a runtime of 1.38 seconds:

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

from errno import ECONNREFUSED
from functools import partial
from multiprocessing import Pool
import socket

NUM_CORES = 4


def ping(host, port):
    try:
        socket.socket().connect((host, port))
        print(str(port) + " Open")
        return port
    except socket.error as err:
        if err.errno == ECONNREFUSED:
            return False
        raise


def scan_ports(host):
    p = Pool(NUM_CORES)
    ping_host = partial(ping, host)
    return filter(bool, p.map(ping_host, range(1, 65536)))


def main(host=None):
    if host is None:
        host = "127.0.0.1"

    print("\nScanning ports on " + host + " ...")
    ports = list(scan_ports(host))
    print("\nDone.")

    print(str(len(ports)) + " ports available.")
    print(ports)


if __name__ == "__main__":
    main()
\$\endgroup\$
  • \$\begingroup\$ Wow. Amazing performance difference! Thanks for all your comments. I've got some Googling to do to completely figure out how your code works, but this will definitely help me learn the language better. \$\endgroup\$ – asteri Jan 3 '14 at 14:15
  • \$\begingroup\$ Can you explain your usage of the filter() function? I don't get why your first argument is bool. It seems like the function is supposed to take another function as the first argument, or None to just purge False (which seems like what you're trying to do). Would passing in None have worked just as well? \$\endgroup\$ – asteri Jan 3 '14 at 14:45
  • \$\begingroup\$ Interestingly, your version actually runs a lot slower on my PC. The performance benefits may only be on *nix. \$\endgroup\$ – asteri Jan 3 '14 at 15:01
  • \$\begingroup\$ Forking is cheap on *nix, expensive on Windows. Vice versa for threads. Also make sure NUM_CORES accurately reflects how many physical cores on the machine. \$\endgroup\$ – wting Jan 3 '14 at 18:21
  • \$\begingroup\$ From map(ping_host(range(1, 65536))) we get an example list [False, 2, False, 4, ...]. filter(bool, list) will apply bool() to each element in the list and only keep those that evaluate to True. None, False, 0, [], '' evaluates to False, everything else evaluates to True. Thus filter(bool, [None, 2, False, 4]) results in the list [2, 4]. \$\endgroup\$ – wting Jan 3 '14 at 18:26
2
\$\begingroup\$

Apart from wting's answer I'd add that you call main() and then scan_ports() when you could've called scan_ports directly in the if __name__ == '__main__': block.

\$\endgroup\$
  • 1
    \$\begingroup\$ It's preferable that Jeff calls main() and implements logic within that function. This allows for better use at the interpreter (from scan import main) and unit testing. Guido goes into this a bit more: artima.com/weblogs/viewpost.jsp?thread=4829 \$\endgroup\$ – wting Jan 3 '14 at 18:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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