THREADWORKER
hamra007
Messages postés
10
Date d'inscription
lundi 10 avril 2006
Statut
Membre
Dernière intervention
3 avril 2011
-
18 juin 2010 à 10:26
krimog Messages postés 1860 Date d'inscription lundi 28 novembre 2005 Statut Membre Dernière intervention 14 février 2015 - 21 juin 2010 à 09:37
krimog Messages postés 1860 Date d'inscription lundi 28 novembre 2005 Statut Membre Dernière intervention 14 février 2015 - 21 juin 2010 à 09:37
Cette discussion concerne un article du site. Pour la consulter dans son contexte d'origine, cliquez sur le lien ci-dessous.
https://codes-sources.commentcamarche.net/source/51918-threadworker
https://codes-sources.commentcamarche.net/source/51918-threadworker
21 juin 2010 à 09:37
@Coq : L'indulgence, ce n'est pas dire que mon programme est parfait quand il ne l'est pas. C'est juste de ne pas dire que ce que j'ai fait c'est de la m****. Donc ton commentaire reste indulgent envers moi.
De plus, toutes les remarques sont bonnes à prendre. Que ce soit concernant le code source en question, ou ma façon de coder en général.
Il est vrai que je n'ai pas du tout pensé à faire de la programmation Thread-safe. Quant à tes remarques diverses, le "static" est effectivement un oubli) et il est vrai que je devrais transformer mes ArgumentException en InvalidOperationException. Enfin, oui, ce serait mieux de créer une méthode appelée à la fois lors du tick du timer et au lancement de la boucle plutôt que d'appeler l'événement directement.
Donc, au final, merci pour ces remarques. J'effectuerai les modifications quand j'aurai un peu de temps.
19 juin 2010 à 17:52
J'ai jeté un oeil à la classe ThreadWorker et j'ai quelques remarques (du coup c'est raté pour l'indulgence, mais il vaut mieux ça que d'avoir des problèmes en prod ;-)).
Thread-safety :
- tu devrais peut être préciser qu'une instance de la classe ThreadWorker en elle même ne doit pas être manipulée par plusieurs threads en même temps (sinon tu as des conditions de race, notamment dans la méthode Start)
- thread-safe ou pas, tu dois synchroniser tous les accès aux champs partagés par plusieurs instances de la classe : l'utilisateur externe ne peut pas le faire sans synchroniser lui même les accès à toutes les instances (ce qui est en général impossible) et de toute façon il n'a même pas conscience de devoir le faire :-)
Dans le cas présent il faut que tu synchronises les accès à _twList car List<T> n'est pas thread-safe.
Dans le cas contraire tu pourras te retrouver avec quelques problèmes en cas d'exécution de Add/Remove lors d'une énumération.
- pour les mêmes raisons, tu devrais peut être changer l'implémentation de la propriété RunningThreadWorkers pour qu'elle retourne une copie de la liste et non pas directement la référence à la liste interne : tu ne pourras pas synchroniser les accès externes, sauf si tu gères la synchro interne via ICollection.SyncRoot et que l'utilisateur dans la classe s'en sert aussi (mais c'est quasiment sûr qu'il oubliera)
Du coup tu as maintenant un problème de performance en cas d'utilisation brutale (polling) de cette propriété, et donc un choix à faire : est-il vraiment utile de l'exposer ? (si oui, il faudra peut être considérer la transformer en méthode, pour mettre plus en avant le fait qu'un appel à un certains coût).
Divers :
- la propriété RunningThreadWorkers devrait être marquée static (là je pense que c'est un simple oubli)
- dans la méthode Start tu lèves 2 exceptions "ArgumentException", alors que la méthode ne prend pas de paramètres. Dans le cas présent je pense qu'il faudrait plutôt utiliser InvalidOperationException (c'est sans doute aussi le cas pour les levées de UnauthorizedAccessException)
- "if (_startNow) _timer_Elapsed(_timer, new EventArgs());" : en général j'évite d'écrire ce genre de chose et préfère déporter le code du gestionnaire d'évènement "_timer_Elapsed" dans une méthode qui sera appelée des 2 endroits : on ne s'attend pas vraiment à ce qu'un gestionnaire d'évènement soit appelé autrement que par la levée de l'évènement.
18 juin 2010 à 10:26
c'est très intéressant