2009-06-12 10 views
3

Mon approche d'une interface graphique réactive avec un processus d'arrière-plan est-elle correcte? Si non, s'il vous plaît s'il vous plaît critiquer et offrir des améliorations. En particulier, indiquez quel code pourrait potentiellement souffrir d'une situation de blocage ou de course.Filetage C# et Windows Forms

Le thread de travail doit pouvoir être annulé et signaler sa progression. Je n'ai pas utilisé BackgroundWorker parce que tous les exemples que j'ai vus ont le code de processus sur le formulaire lui-même, plutôt qu'un objet séparé. J'ai pensé hériter le LongRunningProcess pour BackgroundWorker mais j'ai pensé que cela introduirait des méthodes inutiles sur l'objet. Idéalement, je préférerais ne pas avoir de référence au processus ("_lrp"), mais je ne vois pas comment il serait possible d'annuler le processus, sauf si j'ai un événement sur le LRP qui vérifie un drapeau sur l'appelant, mais cela semble inutilement complexe et peut-être même faux.

Windows Form (Edit: déplacé * .EndInvoke appels à la fonction de rappel)

public partial class MainForm : Form 
{ 
    MethodInvoker _startInvoker = null; 
    MethodInvoker _stopInvoker = null; 
    bool _started = false; 

    LongRunningProcess _lrp = null; 

    private void btnAction_Click(object sender, EventArgs e) 
    { 
     // This button acts as a Start/Stop switch. 
     // GUI handling (changing button text etc) omitted 
     if (!_started) 
     { 
      _started = true; 
      var lrp = new LongRunningProcess(); 

      _startInvoker = new MethodInvoker((Action)(() => Start(lrp))); 
      _startInvoker.BeginInvoke(new AsyncCallback(TransferEnded), null); 
     } 
     else 
     { 
      _started = false; 
      _stopInvoker = new MethodInvoker(Stop); 
       _stopInvoker.BeginInvoke(Stopped, null); 
     } 
    } 

    private void Start(LongRunningProcess lrp) 
    { 
     // Store a reference to the process 
     _lrp = lrp; 

     // This is the same technique used by BackgroundWorker 
     // The long running process calls this event when it 
     // reports its progress 
     _lrp.ProgressChanged += new ProgressChangedEventHandler(_lrp_ProgressChanged); 
     _lrp.RunProcess(); 
    } 

    private void Stop() 
    { 
     // When this flag is set, the LRP will stop processing 
     _lrp.CancellationPending = true; 
    } 

    // This method is called when the process completes 
    private void TransferEnded(IAsyncResult asyncResult) 
    { 
     if (this.InvokeRequired) 
     { 
      this.BeginInvoke(new Action<IAsyncResult>(TransferEnded), asyncResult); 
     } 
     else 
     { 
      _startInvoker.EndInvoke(asyncResult); 
      _started = false; 
      _lrp = null; 
     } 
    } 

    private void Stopped(IAsyncResult asyncResult) 
    { 
     if (this.InvokeRequired) 
     { 
      this.BeginInvoke(new Action<IAsyncResult>(Stopped), asyncResult); 
     } 
     else 
     { 
      _stopInvoker.EndInvoke(asyncResult); 
      _lrp = null; 
     } 
    } 

    private void _lrp_ProgressChanged(object sender, ProgressChangedEventArgs e) 
    { 
     // Update the progress 
     // if (progressBar.InvokeRequired) etc... 
    } 
} 

processus d'arrière-plan:

public class LongRunningProcess 
{ 
    SendOrPostCallback _progressReporter; 
    private readonly object _syncObject = new object(); 
    private bool _cancellationPending = false; 

    public event ProgressChangedEventHandler ProgressChanged; 

    public bool CancellationPending 
    { 
     get { lock (_syncObject) { return _cancellationPending; } } 
     set { lock (_syncObject) { _cancellationPending = value; } } 
    } 

    private void ReportProgress(int percentProgress) 
    { 
     this._progressReporter(new ProgressChangedEventArgs(percentProgress, null)); 
    } 

    private void ProgressReporter(object arg) 
    { 
     this.OnProgressChanged((ProgressChangedEventArgs)arg); 
    } 

    protected virtual void OnProgressChanged(ProgressChangedEventArgs e) 
    { 
     if (ProgressChanged != null) 
      ProgressChanged(this, e); 
    } 

    public bool RunProcess(string data) 
    { 
     // This code should be in the constructor 
     _progressReporter = new SendOrPostCallback(this.ProgressReporter); 

     for (int i = 0; i < LARGE_NUMBER; ++i) 
     { 
      if (this.CancellationPending) 
       break; 

      // Do work.... 
      // ... 
      // ... 

      // Update progress 
      this.ReportProgress(percentageComplete); 

      // Allow other threads to run 
      Thread.Sleep(0) 
     } 

     return true; 
    } 
} 
+0

Voté pour fermer depuis "Veuillez critiquer" ne ressemble pas beaucoup à une question. –

Répondre

1

I comme la séparation du processus d'arrière-plan dans un objet séparé. Cependant, j'ai l'impression que votre thread d'interface utilisateur est bloqué jusqu'à ce que le processus d'arrière-plan soit terminé, car vous appelez BeginInvoke et EndInvoke dans le même gestionnaire de boutons.

MethodInvoker methodInvoker = new MethodInvoker((Action)(() => Start(lrp))); 
IAsyncResult result = methodInvoker.BeginInvoke(new AsyncCallback(TransferEnded), null); 
methodInvoker.EndInvoke(result); 

Ou est-ce qu'il me manque quelque chose?

+0

Oui, je pense que vous avez raison à ce sujet. Je ne sais pas comment j'ai manqué cela initialement mais je vais déplacer le code à la méthode de rappel. – ilitirit

1

Je suis un peu confus par votre utilisation de MethodInvoker.BeginInvoke(). Y at-il une raison pour laquelle vous avez choisi d'utiliser ceci au lieu de créer un nouveau thread et d'utiliser Thread.Start() ...? Je crois que vous pourriez bloquer votre thread UI parce que vous appelez EndInvoke sur le même thread que BeginInvoke. Je dirais que le motif normal est d'appeler EndInvoke sur le thread de réception. C'est certainement vrai avec les opérations d'E/S asynchrones - des excuses si cela ne s'applique pas ici. Vous devriez facilement être en mesure de déterminer si votre thread d'interface utilisateur est bloqué jusqu'à ce que le LRP se termine de toute façon. Enfin, vous comptez sur un effet secondaire de BeginInvoke pour démarrer votre LRP sur un thread de travail à partir du pool de threads géré. Encore une fois, vous devriez être sûr que c'est votre intention. Le pool de threads inclut la sémantique des files d'attente et fait un excellent travail lorsqu'il est chargé avec un grand nombre de processus de courte durée. Je ne suis pas sûr que ce soit un si bon choix pour les processus de longue durée. Je préférerais utiliser la classe Thread pour démarrer votre thread à long terme.

Aussi, bien que je pense que votre méthode de signalisation du LRP pour l'annuler fonctionne, j'utilise normalement un ManualResetEvent à cet effet. Vous n'avez pas à vous soucier de verrouiller un événement pour vérifier son état.

+0

Pour être honnête, je sais qu'il y a une raison pour laquelle je n'utilise pas Thread.Start, je ne peux pas me souvenir de ce qu'il est en ce moment. Il se pourrait que la raison ne s'applique plus, alors je vais essayer une implémentation différente et comparer les résultats. – ilitirit

1

Vous pouvez rendre votre _cancellationPending volatile et éviter le verrouillage. Pourquoi appelez-vous Stop dans un autre thread?

Vous devriez changer d'appeler votre événement méthode pour éviter la condition de course:

protected virtual void OnProgressChanged(ProgressChangedEventArgs e) 
{ 
    var progressChanged = ProgressChanged; 
    if (progressChanged != null) 
     progressChanged(this, e); 
} 

Si le travailleur de fond correspond, vous n'avez pas à recoder;)

+0

J'appelle Stop dans un thread séparé car j'ai besoin de savoir quand il s'est arrêté (le callback), mais je vais m'éloigner de cette méthode et utiliser un ProcessCompletedEvent ou quelque chose de similaire. Je peux utiliser BackgroundWorker, mais je devrai recoder la méthode Worker pour être compatible avec l'événement bw.DoWork. Je vais essayer plusieurs possibilités. – ilitirit

+0

Je pense qu'il y a un problème ... Votre méthode d'arrêt/rappel n'attend pas la fin de votre lrp. Mais si vous changez, ça va. Utilisez le travailleur, c'est plus sûr que les modèles faits maison. –

0

Comme Guillaume a affiché, vous avez Dans la méthode OnProgressChanged, je ne crois pas que la réponse fournie soit une solution. Vous avez toujours besoin d'un objet de synchronisation pour le gérer.

private static object eventSyncLock = new object(); 

protected virtual void OnProgressChanged(ProgressChangedEventArgs e) 
{ 
    ProgressChangedEventHandler handler; 
    lock(eventSyncLock) 
    { 
     handler = ProgressChanged; 
    } 
    if (handler != null) 
     handler(this, e); 
} 
+0

Le code que j'utilise pour OnProgressChanged provient plus ou moins de la classe BackgroundWorker et ne vérifie pas les conditions de concurrence. Peut-être qu'il y a quelque chose qui me manque cependant. – ilitirit

+0

Mon code est correct, vous n'avez pas besoin d'un verrou: http://blogs.msdn.com/ericlippert/archive/2009/04/29/events-and-races.aspx –

+0

Eh bien, je suppose que oui se termine toujours avec une condition de concurrence, car le lien que vous avez publié indique "Ce code a une condition de concurrence qui entraîne un comportement incorrect". Mon code garantit que vous obtenez la valeur la plus à jour lors de l'affectation au gestionnaire. http://www.yoda.arachsys.com/csharp/events.html – scottm

0

Vous pouvez utiliser un BackgroundWorker et déplacer votre code de travail en dehors de la classe Form. Avoir votre classe Travailleur, avec sa méthode de travail. Laissez Work prendre un BackgroundWorker en tant que paramètre et surchargez la méthode Work avec une signature non-BackgroundWorker, qui envoie null à la première méthode.

Ensuite, dans votre formulaire, utilisez un BackgroundWorker qui a ProgressReporting, et dans votre travail (BackgroundWorker bgWorker, params objet [] otherParams), vous pouvez inclure des déclarations telles que:

if(bgWorker != null && bgWorker.WorkerReportsProgress) 
    { 
     bgWorker.ReportProgress(percentage); 
    } 

... et de même inclure des chèques pour CancellationPending.

Ensuite, dans votre code Forms, vous pouvez gérer les événements. Définissez d'abord bgWorker.DoWork += new DoWorkEventHandler(startBgWorker);, où cette méthode démarre votre méthode Worker.Work en passant en argument bgWorker. Ceci peut être lancé à partir d'un événement de bouton, appelé bgWorker.RunWorkerAsync. Un deuxième bouton d'annulation peut alors appeler bgWorker.CancelAsync, qui sera alors attrapé dans votre section où vous avez coché CancelPending.

En cas de succès ou d'annulation, vous allez gérer l'événement RunWorkerCompleted, dans lequel vous vérifiez si le travailleur a été annulé. Ensuite, si ce n'était pas le cas, vous supposez que cela a été un succès et suivez cette route. En surchargeant la méthode Work, vous pouvez la réutiliser du code qui ne se soucie pas de Forms ou de ComponentModel.

Et, bien sûr, vous implémentez l'événement progresschanged sans avoir besoin de réinventer la roue sur celui-ci. ProTip: ProgressChangedEventArgs prend un int, mais ne le force pas à un maximum de 100. Pour signaler un pourcentage de progression plus fin, passez un argument avec un multiplicateur (disons 100), donc 14,32% sera un progrès de 1432. Alors vous pouvez formater l'affichage, ou remplacer votre barre de progression ou l'afficher en tant que champ de texte. (tous avec conception DRY-friendly)