String für einen Thread "locken"

Hallo zusammen,

ich schreibe grad an einer Anwendung. Dieser möchte ich die Arbeit mit Multithreading verkürzen. An und für sich werden auch Threads erstellt und deren Aufgabe (die bei jedem Thread die gleich ist) erfüllt. Um Ihre Aufgabe zu erfüllen, bekommen sie aus einer Textdatei einen String. Für jede sich in der Textdatei befindlich Zeile wird ein Thread gestartet und die auszuführende Methode hat als Überlagerung einen String.

So, das funktioniert auch schon fast, bis auf ein kleines Problem. Angenommen es sind 3 Zeile in der Textdatei Param1, Param2, Param3. Das Ergebnis, also wenn die Methode ausgeführt wurde, schreibe ich in eine Logdatei etwas wie "Param1 abgearbeitet, Param2 abgearbeitet" usw.. Tja, was steht nun in der Logdatei, wenn alles ausgeführt wurde? ....Richtig! "Param3 abgearbeitet, Param1 abgearbeitet, Param3 abgearbeitet".

Wo ist Param2 hin? Und Warum beginnt er mit Param3 ?

Achso, die Params bekommt der jeweilige Thread aus einem Array (Ja, ich starte eigentlich bei 0), das Programm nur nicht 😉

Das ist eben Multi-Threading. Jeder Thread bekommt eine Zeile. Welcher Thread zuerst ausgeführt wird und damit das Ergebnis schreibt ist "zufällig".

Wenn du die Reihenfolge erhalten willst, dann ist Threading genau das NICHT, was du verwenden solltest.

Zudem verkürzt du damit nix. Im Gegenteil. Thread erstellen ist nicht umsonst. Nur auf einem Mehr-Prozessor-System kannst du theoretisch Geschwindigkeitsvorteile erzielen. Das aber auch nur, wenn die Prozessoren wirklich "rechnen". Für I/O-Operationen bringt es nichts.

Klassische Race-Condition.
Wahrscheinlich hast du eine Variable, die dem Thread angibt, welche Zeile er sich nehmen soll.
Jetzt startest du zwei Threads, der erste inkrementiert die Variable, wird vom zweiten unterbrochen, der inkrementiert auch, dann (Reihenfolge egal) indizieren sie erst das Element. Dadurch hast du jetzt ein Element übersprungen und beide arbeiten auf dem gleichen.
Um dies zu vermeiden gibt es zwei Möglichkeiten:

1. du synchronisierst den Zugriff auf die Variable und das Array per lock oder
2. (besser) ausschließlich der Steuer-Thread bestimmt, welchen String der neu erstellte Thread abarbeitet, liest diesen aus und startet den Thread damit als Parameter (ParametrizedThreadStart).

Ok, hab mich da vielleicht nicht ganz korrekt ausegdrückt 🙂

Also, das was als Überladung aus dem Textfile kommt, wird nur für die weitere Verarbeitung (das was der Thread machen soll) benötigt.

Der Thread macht dann durchaus noch einige mehr und die Reihenfolge ist mir im Prinzip egal. Das Problem ist ja, das er den 1. und den letzten Wert aus dem Textfiel nimmt, die zweite Zeile wird schluckt.

EDIT: Ok, da war jemand schneller oder hat eher angefangen zu tippen =)

Werde es erstmal per ParametrizedThreadStart (wie Borg vorgeschlagen hat) und melde mich dann zurück!

Aber auch danke an svenson

Original von Borg
Klassische Race-Condition.
Wahrscheinlich hast du eine Variable, die dem Thread angibt, welche Zeile er sich nehmen soll. IMHO der kassische C# Bug, der lokale, threaded, anonyme Methoden innerhalb einer Schleife betrifft.
Kann man sehr einfach reproduzieren:
"Funktioniert" gibt a, b und c in irgendeiner Reihenfolge. "FunktioniertNicht" gibt meistens c, c und c, da das Feld der compiler generierten Klasse für die anonyme Methode immer wieder überschrieben wird.

static Random r = new Random();

static void Main(string[] args)
{
  string[] lines = { "a", "b", "c" };
  Funktioniert(lines);
  FunktioniertNicht(lines);
}

private static void WaitForAll(IEnumerable<Thread> threads)
{
  foreach(Thread thread in threads)
  {
    if ((thread.ThreadState & ThreadState.Running) == ThreadState.Running)
      thread.Join();
  };
}

private static void FunktioniertNicht(string[] lines)
{
  List<Thread> threads = new List<Thread>(lines.Length);

  foreach (string line in lines)
  {
    ThreadStart ts = delegate()
    {
      Thread.Sleep(r.Next(200));// zufällige dummy-latenz
      Console.WriteLine(line);
    };

    Thread thread = new Thread(ts);
    thread.Start();

    threads.Add(thread);
  }

  // warte bis alle färtsch sind
  WaitForAll(threads);
}

private static void Funktioniert(string[] lines)
{
  List<Thread> threads = new List<Thread>(lines.Length);

  foreach (string line in lines)
  {
    threads.Add(RunThreaded(line));
  }

  // warte bis alle färtsch sind
  WaitForAll(threads);
}


private static Thread RunThreaded(string line)
{
  ThreadStart ts = delegate()
  {
    Thread.Sleep(r.Next(200));// zufällige dummy-latenz
    Console.WriteLine(line);
  };
  Thread result = new Thread(ts);
  result.Start();
  return result;
}

Für multi-threaded-taugliche "Zähler" sei noch die Verwendung von InterlockedIncrement/Decrement erwähnt.

Ist in meinen Augen aber kein Bug, sondern falsch programmiert, da sich alle Threads eine (relativ) globale Variable line teilen. Dies kann natürlich nicht gut gehen.
Im anderen Beispiel hat ja jeder Thread eine lokale Variable (entspricht also dem o.g. ParametrizedThreadStart) und alles ist schick.

EDIT: Schon, wenn du die Schleife so umschriebst, sollte es funktionieren:

foreach (string line in lines)
  {
    ParametrizedThreadStart ts = delegate( object _line )
    {
      Thread.Sleep(r.Next(200));// zufällige dummy-latenz
      Console.WriteLine(_line as string);
    };

    Thread thread = new Thread(ts);
    thread.Start( line );

    threads.Add(thread);
  }

Ok, wie bereits im Edit erwähnt, werd ich mal versuchen das ganze Parametrized zu verwenden. Könnte vielleicht etwas dauern, da ich zum Einen noch C#-Anfänger und mich zum Anderen erst seit einpaar Tagen mit Threading auseinandersetze

Original von Borg
Im anderen Beispiel hat ja jeder Thread eine lokale Variable (entspricht also dem o.g. ParametrizedThreadStart) und alles ist schick.

Jo. Man sollte bei Threads den Zugriff auf gemeinsame Daten wenn möglich eliminieren. Denn bei diesem Zugriff beginnen Threads erst lästig zu werden. Also dem Thread gleich den Inhalt der Zeile zu futtern geben.

Original von Borg
Ist in meinen Augen aber kein Bug, sondern falsch programmiert, da sich alle Threads eine (relativ) globale Variable line teilen. Dies kann natürlich nicht gut gehen.
Im anderen Beispiel hat ja jeder Thread eine lokale Variable (entspricht also dem o.g. ParametrizedThreadStart) und alles ist schick. Ooops, wollte Bug in Anführungszeichen setzen.
Der Compiler kann ja schlecht wissen, dass es hier asynchrone Calls sind und er wird unwissend drauflosoptimieren. 😉

So, die Änderung brachte mich wieder zu einem Fehler, den ich bereits in der Vergangenheit hatte. Ich bekomme die Exception:

BeginGetRequestStream/BeginGetResponse kann nicht erneut aufgerufen werden, wenn ein vorheriger Aufruf noch verarbeitet wird.

Grund ist mir soweit klar, nur nicht wie ich das am schönsten/besten behebe

Momentane Methode:

    internal static void readURLs()
    {
      try
      {
        string sLine = "";
        int sumURLs = 0;

        StreamReader objReader = new StreamReader(URLFile);

        //Debug
        logging.writeLog("URLFile in Fire_It: " + URLFile, "error");

        ArrayList arrText = new ArrayList();

        while (sLine != null)
        {
          sLine = objReader.ReadLine();
          if (sLine != null && sLine != "")
          {
            arrText.Add(sLine);
          }
        }

        objReader.Close();

        sumURLs = arrText.Count;

        List<Thread> threads = new List<Thread>(sumURLs);

        foreach (string URL in arrText)
        {
          System.Threading.ParameterizedThreadStart ts = delegate(object _URL)
          {
            Fire.split_uri(URL, UserAgent);
          };

          Thread thread = new Thread(ts);
          thread.Start(URL);

          threads.Add(thread);
        }

        WaitForAll(threads);
        
      }

      catch (Exception exc)
      {
        //Log the Exception
        logging.writeLog(exc.ToString(), "error");
      }

      finally
      {
        logging.writeLog("Threads ordnungsgemäß beendet!", "info");
      }
    }

Mit Lock(this) kannst du doch den Durchlauf ordnunggemäß gezielt durchlaufen.

Jedoch willst du nicht andere Threads warten lassen. Also würde ich das Array mit den URLs gegenprüfen und so die Ordnung "wiederherstellen". Wenn 2x Param3 auftaucht ist das definitiv ein Bug.

Man kann zwar nicht erkennen, wo der Fehler auftritt, aber ich vermute mal, dass alle deine Threads wieder das gleiche Objekt (Instanz) benutzen...

Naja, ob jetzt noch irgendein Parameter doppelt "gelesen" wird, kann ich durch besagt Exception nocht nicht sagen 😉

Hallo zusammen,

habe den Kern dieses Threads nicht nachvollzogen, möchte aber zu folgendem Punkt eine BBemerkung machen:

Lock(this)

Ich habe gelesen, dass das überhaupt nicht gut ist, da es hier die Referenz des Objektes als Lock-Kriterium heranzieht. Es kann aber sein, dass die Referenz sich ändert, dann wäre das Lock hinfällig.

Besser: Eine Konstante erstellen (z.B. String) und diesen nehmen. Oder wenn ein "readonly" Objekt gelockt werden soll, dann besser gleich dieses Objekt heranziehen.

Gruß
Norman-Timo

Wo tritt denn der Fehler überhaupt auf? Bei ReadLine()?

Die Exc. erfolgt an der markierten Stelle

try
      {
        //Rebuild the URI to avoid Exceptions or other kinds of Errors
        Uri cleanURL;
        Uri.TryCreate(URL, UriKind.Absolute, out cleanURL);
      }
      catch (WebException webexception)
      {
        logging.writeLog("Precompile throw Exeption: " + Convert.ToString(webexception) + "\r\n", "error");
        logging.writeLog("Fehler beim Erstellen der URI, bitte URLs in der urls.txt prüfen!\r\n", "error");

        return;
      }

      if (wreq.Method == "GET")
      {
        //Fire the Request
        HttpWebResponse wres = (HttpWebResponse)wreq.GetResponse();
      }
      else
      {
        //Fire the Request
        HttpWebResponse wres = (HttpWebResponse)wreq.GetResponse(); <------

        wres.Close();
      }

Das was in der gesamten Klasse passiert, aus der dieser Snippet ist, soll von jedem Thread mit der entsprechenden URL gemacht werden. Wenn ich aber lock verwende, würd ich ja eine Wartschlange erzeugen und könnt mir das Threading ja gewissermaßen sparen oder seh ich das falsch?

Vermutlich ist wreq wieder ein Objekt, auf welches mehrere Threads zugreifen. Jeder Thread muss sich sein eigenes Request-Objekt erzeugen.

Original von norman_timo
Hallo zusammen,

habe den Kern dieses Threads nicht nachvollzogen, möchte aber zu folgendem Punkt eine BBemerkung machen:

Lock(this)

Ich habe gelesen, dass das überhaupt nicht gut ist, da es hier die Referenz des Objektes als Lock-Kriterium heranzieht. Es kann aber sein, dass die Referenz sich ändert, dann wäre das Lock hinfällig.

Besser: Eine Konstante erstellen (z.B. String) und diesen nehmen. Oder wenn ein "readonly" Objekt gelockt werden soll, dann besser gleich dieses Objekt heranziehen.

Gruß
Norman-Timo

Bei großen Softwarearchiekturen ist das sicher richtig. Aber wie gesagt, er soll das Array durchlaufen und die Threads für die einzelnen URL Einträge starten.

Ja, jetzt tu ich comprendre. Für das Schreiben wäre ein Lock angemessen mit Gegenprüfung zum Array. Dann wird auch die Liste ordnungsgemäß wiedergegeben.

System.Threading.ParameterizedThreadStart ts = delegate(object _URL)
          {
            Fire.split_uri(URL, UserAgent);
          };

Dieser Methodenaufruf gilt aber doch pro Thread oder nicht? split_uri() ruft dann im weiteren Verlauf "Request.FireRequest(URI, UserAgent, username, password);" auf und in der Mehtode FireRequest, der Klasse Request wird der WebRequest (wreq) erzeugt.

Hm, verstehe nicht so ganz wo das Problem sitzt 🙁

Exakt, gilt pro Aufruf. Normalerweise brauchst du dich jetzt nicht mehr um die Aufruffolge kümmern, wie ich sehen kann. Nur der doppelte Aufruf ist das Problem. Ist das der komplette Code von dir, denn wir hier im Thread sehen?

Nein, das ist nicht der komplette Code. Der gesamte aus allen 5 Klassen sind ca. 460 Zeilen. Welche Stelle wäre denn noch interessant?

Original von LordHexa
Hm, verstehe nicht so ganz wo das Problem sitzt 😦

Es gibt auch keins. Dieser ganze Lock-Kram ist mit deiner Umstellung auf ParametrizedThreadStart nicht mehr notwendig. Die Threads sind damit vollkommen isoliert, zumindest was den Eingangsparameter angeht. Dein Exception-Problem scheint aber wieder gemeinsamer Datenzugriff zu sein (wreq).

Es wäre interessant zu sehen, wo "wreq" definiert ist, wann und wer das Objekt erzeugt.

Original von LordHexa

System.Threading.ParameterizedThreadStart ts = delegate(object _URL)  
          {  
            Fire.split_uri(URL, UserAgent);  
          };  

Dieser Methodenaufruf gilt aber doch pro Thread oder nicht? Du hast einen Parameter _URL, aber du benutzt in der anonymen Methode nur URL.
Also greifst du immer noch auf den gleichen String zu.

System.Threading.ParameterizedThreadStart ts = delegate(object _URL)
          {
            Fire.split_uri(Convert.ToString(_URL), UserAgent);
          };

So besser? Wenn nicht, ich bekomm langsam Kopfweh =)

Was lernt uns das: Keine vorangestellten Unterstriche für Variablennamen verwenden. Man übersieht sie zu leicht....

@svenson

Bei mit _ geflaggten Variablen (Klassenebene) ein this. verwenden... dann geht der _ auch nicht so schnell unter.