Tweet
Hallo,
im Arikel "Get what you want" in Ausgabe 08/2011 des dot.net Magazins, Seite 54* sind Hr. Manfred Steyer ein paar Fehler bzw. Missgeschicke unterlaufen. Aber der Reihe nach.
Abschnitt: Implementierung eines Publish/Subscribe-Service:
Hier finde ich die Bezeichnung "Attribut" unpassend. Im .net-Umfeld ist üblicherweise mit Attribut das gemeint: Attribute.
Auch aus OOP-Sicht ist es nicht ganz üblich denn mit einem Attribut wird eher eine Eigenschaft und nicht ein Feld assoziiert.
Die Bezeichnung "Feld" wäre korrekt und treffender.
Welche Concurrency-Probleme? Ein Hinweis in Klammer würde genügen um stichwortartig die potentiellen Probleme beim Namen zu nennen. Mir sind sie schon klar welche auftreten könn(t)en, aber ich denke nicht jeder Leser ist mit Multi-Threading vertraut. Daher sollte darauf hingewiesen werden.
Dass ein Kopie übergeben wird hat auch eher den Grund dass die Auflistung während der Enumeration nicht verändert werden darf (InvalidOperationException). Abgesehen von diesem Problem treten selbst beim gleichzeitigen Zugriff mehrere Threads keine Probleme mit dem Iterator selbst auf - korrekte Implementierung vorausgesetzt und diese ist bei List<T> gegeben.
Listing 5:
Eine Monitor-Synchronisation (lock) über die gesamte ist nicht sinnvoll und wird auch nirgends so empfohlen. Zu locken ist nur der Zugriff auf eine geteilte Ressource (hier das Dictionary callbacks und die List tempCallbacks). Da aber der Artikel wegen der Verwendung von Task auf .net 4.0 abziehlt wäre es gleich besser ein ConcurrentDictionary<K, V> zu verwenden. Zu locken ist dann nur noch das Erstellen der List tempCallbacks im else-Zweig und tempCallbacks.Add. Eine Verwendung von einem ConcurrentBag<T> wäre zwar möglich, halte ich aber nicht für unbedingt angebracht.
Listing 6:
Die List failed wird deklariert und initialisiert aber sonst nicht weiter verwendet. Das könnte aus dem Code entfernt werden.
Anstatt in der foreach-Schleife für jedes Callback einen wertvollen CPU-Thread, durch die Erstellung von Tasks für jeden Callback, verschwenden wäre es schon besser die foreach-Schleife in einem Task laufen zu lassen. Da die Callback-Methode als IsOneWay=true deklariert ist reicht das wenn überhaupt eine nebenläufiger Vorgang notwendig ist.
Im Fehlerfall einfach das Callback zu entfernen ist eine Möglichkeit, aber bei weitem nicht die einzige. Auf dies hätte hingewiesen werden sollen. ZB dass auch Retry, etc. möglich wären.
Für das lock gilt selbiges wie schon oben erwähnt. Das einzig zu lockende wäre das Erstellen der Kopie der Liste und selbst das kann durch Interlocked.CompareExchange<T> erspart werden. Der "globale" Lock ist überhaupt nicht notwendig.
BTW: wenn ganze Methoden gelockt werden sollen gibt es auch das [MethodImpl(MethodImplOptions.Synchronized)]-Attribut (hier passt die Bezeichnung Attribut (sic!)).
Abschnitt: Konfiguration:
Hier wäre noch der Hinweis wie und wo der Service gehostet werden kann praktisch gewesen bzw. das würde das Thema abrunden. ZB bleibt so die Frage offen ob der Service im IIS gehostet werden kann.
Abschnitt: Implementierung des zu benachrichtigenden Clients:
Die Schnittstelle IPublish wurde bisher weder deklariert noch erwähnt. Gemeint ist wahrscheinlich IPublisher.
Das Interface wird nicht nachgebildet sondern implementiert. Das ist ein fundamentaler Unterschied!
Listing 8:
Wenn die Console.ForegroundColor gesetzt wird sollte diese auch wieder rückgesetzt werden, ansonsten ist jede folgende Ausgabe bis zum erneutem Setzen in derselben Farbe.
Abschnitt: Bewertung und Verbesserungsmöglichkeiten:
Auf den nicht unwesentlichen Punkt wenn ein Subscriber ausfällt wird nicht nenneswert eingegangen. Nur der Code in Listing 6 (der catch-Teil) geht indirekt darauf ein.
Mir ist klar dass der Platz begrenzt ist und somit nicht jede Eventualität berücksichtigt werden kann, aber das gezeigte sollte von einem Basta-Speaker und FH-Studienrichtungsverantlichem schon vorbildhaft und in sich korrekt dargelegt werden.
mfG Gü
* der letzte Leserbrief betraf ebenfalls Seite 54
im Arikel "Get what you want" in Ausgabe 08/2011 des dot.net Magazins, Seite 54* sind Hr. Manfred Steyer ein paar Fehler bzw. Missgeschicke unterlaufen. Aber der Reihe nach.
Abschnitt: Implementierung eines Publish/Subscribe-Service:
...Dazu kommt in den einzelnen Methoden das Schlüsselwort lock gemeinsam mit dem statischen Attribut sync zum Einsatz.
Auch aus OOP-Sicht ist es nicht ganz üblich denn mit einem Attribut wird eher eine Eigenschaft und nicht ein Feld assoziiert.
Die Bezeichnung "Feld" wäre korrekt und treffender.
...Um Concurrency-Probleme zu vermeiden, wird eine Kopie der Liste mit den jeweiligen Callbacks und nicht die Liste selbst übergeben.
Dass ein Kopie übergeben wird hat auch eher den Grund dass die Auflistung während der Enumeration nicht verändert werden darf (InvalidOperationException). Abgesehen von diesem Problem treten selbst beim gleichzeitigen Zugriff mehrere Threads keine Probleme mit dem Iterator selbst auf - korrekte Implementierung vorausgesetzt und diese ist bei List<T> gegeben.
Listing 5:
Eine Monitor-Synchronisation (lock) über die gesamte ist nicht sinnvoll und wird auch nirgends so empfohlen. Zu locken ist nur der Zugriff auf eine geteilte Ressource (hier das Dictionary callbacks und die List tempCallbacks). Da aber der Artikel wegen der Verwendung von Task auf .net 4.0 abziehlt wäre es gleich besser ein ConcurrentDictionary<K, V> zu verwenden. Zu locken ist dann nur noch das Erstellen der List tempCallbacks im else-Zweig und tempCallbacks.Add. Eine Verwendung von einem ConcurrentBag<T> wäre zwar möglich, halte ich aber nicht für unbedingt angebracht.
Listing 6:
Die List failed wird deklariert und initialisiert aber sonst nicht weiter verwendet. Das könnte aus dem Code entfernt werden.
Anstatt in der foreach-Schleife für jedes Callback einen wertvollen CPU-Thread, durch die Erstellung von Tasks für jeden Callback, verschwenden wäre es schon besser die foreach-Schleife in einem Task laufen zu lassen. Da die Callback-Methode als IsOneWay=true deklariert ist reicht das wenn überhaupt eine nebenläufiger Vorgang notwendig ist.
Im Fehlerfall einfach das Callback zu entfernen ist eine Möglichkeit, aber bei weitem nicht die einzige. Auf dies hätte hingewiesen werden sollen. ZB dass auch Retry, etc. möglich wären.
Für das lock gilt selbiges wie schon oben erwähnt. Das einzig zu lockende wäre das Erstellen der Kopie der Liste und selbst das kann durch Interlocked.CompareExchange<T> erspart werden. Der "globale" Lock ist überhaupt nicht notwendig.
BTW: wenn ganze Methoden gelockt werden sollen gibt es auch das [MethodImpl(MethodImplOptions.Synchronized)]-Attribut (hier passt die Bezeichnung Attribut (sic!)).
Abschnitt: Konfiguration:
Hier wäre noch der Hinweis wie und wo der Service gehostet werden kann praktisch gewesen bzw. das würde das Thema abrunden. ZB bleibt so die Frage offen ob der Service im IIS gehostet werden kann.
Abschnitt: Implementierung des zu benachrichtigenden Clients:
...der sich auf IPublish abstützt...
Im Zuge dessen wird das Callback-Interface IPublisherCallback nachgebildet.
Listing 8:
Wenn die Console.ForegroundColor gesetzt wird sollte diese auch wieder rückgesetzt werden, ansonsten ist jede folgende Ausgabe bis zum erneutem Setzen in derselben Farbe.
Abschnitt: Bewertung und Verbesserungsmöglichkeiten:
Auf den nicht unwesentlichen Punkt wenn ein Subscriber ausfällt wird nicht nenneswert eingegangen. Nur der Code in Listing 6 (der catch-Teil) geht indirekt darauf ein.
Mir ist klar dass der Platz begrenzt ist und somit nicht jede Eventualität berücksichtigt werden kann, aber das gezeigte sollte von einem Basta-Speaker und FH-Studienrichtungsverantlichem schon vorbildhaft und in sich korrekt dargelegt werden.
mfG Gü
* der letzte Leserbrief betraf ebenfalls Seite 54