Thread Zähler unklar

[pythoner]

Hallo,

ich habe ein Skript geschrieben, welches mehrere ping Prüfungen parallel ausführen soll. Mein Zähler num_threads soll mir anzeigen, wieviele Threads aktiv sind und es sollen maximal 3 Threads erlaubt sein.
Aber der Zähler bleibt nicht zwischen 1 und 3.
Kann mir bitte jemand helfen, warum sich die Variable num_threads über 3 erhöht?

./create_idb_db.py
create idb db file
vm7472 is down, number of threads 1
vm7528 is up, number of threads 4
vm7571 is up, number of threads 6
vm6731 is up, number of threads 12
vm6738 is up, number of threads 12
vm6732 is up, number of threads 12
vm6739 is up, number of threads 12
...
vm7964 is up, number of threads 365
vm7966 is up, number of threads 366
vm0903 is up, number of threads 368
vm7963 is up, number of threads 369
vm7960 is up, number of threads 369
vm7967 is up, number of threads 370

Code: Alles auswählen

def ping(hostname):
    global num_threads, thread_started
    lock.acquire()
    num_threads += 1
    thread_started = True
    lock.release()
    response = os.system("ping -I 10.110.0.1 -c 1 " + hostname + "  >> /dev/null 2>&1")
    if response == 0:
        print hostname, 'is up, number of threads', num_threads
        status = 0
    else:
        response = os.system("ping -I 10.110.0.1 -c 3 " + hostname + "  >> /dev/null 2>&1")
        if response == 0:
            print hostname, 'is up, number of threads', num_threads
            status = 0
        else:
            print hostname, 'is down, number of threads', num_threads
            status = 1
    cp.set(cat,"ping",str(status))
    lock.acquire()
    num_threads -= 1
    lock.release()

cp = ConfigParser.SafeConfigParser()
cp.read(idb_file)

num_threads = 0
thread_started = False
lock = thread.allocate_lock()
for cat in cp.sections():
    device_section, hostname = cat.split(':')
    if device_section == "server":
        try:
            thread.start_new_thread(ping, (hostname,))
            while not thread_started:
                pass
            while (num_threads > 0) and (num_threads < 4):
                pass
        except:
            print "Error: unable to start thread"

[BlackJack]

@pythoner: Schreib das mal ohne das `thread`-Modul, ohne ``global``, und am besten auch gleich ohne `os.system()` neu. Vorher braucht man sich das nicht wirklich anschauen IMHO.

Das zählen der Threads sollte man auch ausserhalb der Threads machen. Denn wenn Du *im* Thread erst hochzählst, dann hast Du ein Zeitfenster in dem gestartete Threads exisieren können, die aber noch nicht im Zähler berücksichtig sind und somit auch nicht verhindern können das mehr als gewollt gestartet werden.

Letztlich solltest Du aber überlegen ob Du überhaupt auf diesem Abstraktionsneveau arbeiten willst statt `concurrent.futures` zu verwenden, oder wenigsten einen Threadpool nicht neu erfinden und `multiprocessing.dummy.Pool` verwenden.

[pythoner]

Hallo,

danke für die Antwort. Ich habe mein Skript jetzt mit threading anstatt thread geschrieben.
Da mein Skript jetzt mit dem folgende Code funktioniert ist das Problem gelöst.

Code: Alles auswählen

import threading

class pingThread(threading.Thread):
    result = {}
    resultLock = threading.Lock()

    def __init__(self, hostname):
        threading.Thread.__init__(self)
        self.hostname = hostname

    def run(self):
        response = os.system("ping -I 10.110.0.1 -c 1 " + self.hostname + "  >> /dev/null 2>&1")
        if response == 0:
            pingThread.resultLock.acquire()
            pingThread.result[self.hostname] = 0
            pingThread.resultLock.release()
            print self.hostname, 'is up'
            return
        else:
            response = os.system("ping -I 10.110.0.1 -c 3 " + self.hostname + "  >> /dev/null 2>&1")
            if response == 0:
                print self.hostname, 'is up'
                pingThread.resultLock.acquire()
                pingThread.result[self.hostname] = 0
                pingThread.resultLock.release()
                return
            else:
                print self.hostname, 'is down'
                pingThread.resultLock.acquire()
                pingThread.result[self.hostname] = 1
                pingThread.resultLock.release()
                return

threads = []
for cat in cp.sections():
    device_section, hostname = cat.split(':')
    if device_section == "server":
        thread = pingThread(hostname)
        threads += [thread]
        ping_status = thread.start()
        print "ping_status: ", ping_status

# Die join()-Methode stellt sicher, dass das Hauptprogramm wartet, bis alle Threads terminiert haben.
for x in threads:
    x.join()

[BlackJack]

@pythoner: Die Namensschreibweise folgt nicht immer dem Style Guide for Python Code.

`result` und das Lock sind an der Stelle globale Variablen die man vermeiden sollte. Wenn die beseitigt sind, dann wird die Klasse noch überflüssiger, denn in der Regel sind Klassen die nur aus der `__init__()` und *einer* Methode bestehen die auch nur einmal aufgerufen wird, nur übermässig kompliziert geschriebene Funktionen. Man kann mit `Thread` auch Funktionen asynchron ausführen, dazu braucht man also keine abgeleitete Klasse.

``threads += [thread]`` ist unnötig ineffizient. Da wird potentiell jedes mal eine neue Liste mit einem Element erstellt, dann eine neue Liste in die der Inhalt der alten Liste und der einelementigen Liste kopiert wird, nur um dann die alte und die einelementige Liste wieder zu verwerfen. An der Stelle wäre ein simples `append()` einfacher und offensichtlicher.

`ping_status` und dessen Ausgabe ist sinnfrei weil `Thread.start()` keinen Rückgabewert hat, da also immer `None` ausgegeben wird.

Die `run()`-Methode enthält einiges an sich wiederholendem Code. Das kann man anders schreiben. Zum Beispiel das Eintragen des Ergebnisses nur einmal am Ende erledigen und das eigentliche Pingen in eine Funktion herausziehen. In der man dann auch gleich noch Abschied von `os.system()` nehmen könnte und das Ergebnis etwas weniger unerwartet gestalten könnte.

Da könnte dann so etwas bei heraus kommen (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf-8
from __future__ import absolute_import, division, print_function
import os
from ConfigParser import SafeConfigParser
from threading import Lock, Thread
from subprocess import call


def _ping(hostname, interface):
    with open(os.devnull, 'wb') as null_file:
        for count in [1, 3]:
            return_code = call(
                ['ping', '-I', interface, '-c', str(count), hostname],
                stdout=null_file,
                stderr=null_file,
            )
            if return_code == 0:
                break
    return return_code != 0


def ping(result, result_lock, hostname, interface):
    is_up = _ping(hostname, interface)
    print(hostname, 'is up' if is_up == 0 else 'is down')
    with result_lock:
        result[hostname] = is_up


def main():
    config_filename = 'test.cfg'
    config = SafeConfigParser()
    config.read(config_filename)

    result = dict()
    result_lock = Lock()
    threads = []
    for section in config.sections():
        device_section, hostname = section.split(':', 1)
        if device_section == 'server':
            thread = Thread(
                target=ping, args=(result, result_lock, hostname, '10.110.0.1')
            )
            thread.start()
            threads.append(thread)

    for thread in threads:
        thread.join()

    print(result)


if __name__ == '__main__':
    main()

Allerdings hast Du jetzt das eigentliche Problem gar nicht mehr in Angriff genommen: Das begrenzen auf eine maximale Anzahl gleichzeitig laufender Threads.

[Sirius3]

Eine Variable namens is_up, die den Wert False hat, wenn sie up ist, auf 0 zu prüfen? Gab es kein Frühstück?

[BlackJack]

@Sirius3: Deshalb steht da ja „(ungetestet)“. Das ist noch die ”Logik” aus dem Original. Ist mir trotz Frühstück durchgerutscht.