Come rendere esclusivo un metodo in un contesto multithread?

Question

Ho un metodo che dovrebbe essere eseguito in modo esclusivo. Fondamentalmente, è un'applicazione multi-thread in cui il metodo viene invocato periodicamente da un timer, ma che potrebbe anche essere attivato manualmente da un'azione dell'utente.

Facciamo un esempio:

1. Il timer scade, quindi il metodo è chiamato. L'operazione potrebbe richiedere alcuni secondi.

2. Subito dopo, l'utente fa clic su alcuni pulsante, che dovrebbe attivare il stesso compito: BAM. Non fa niente poiché il metodo è già in esecuzione.

Ho usato la seguente soluzione:

public void DoRecurentJob()
{
    if(!Monitor.TryEnter(this.lockObject))
    {
        return;
    }

    try
    {
        // Do work
    }
    finally 
    {
        Monitor.Exit(this.lockObject);
    }
}

Dove lockObject è dichiarato così:

private readonly object lockObject = new object();

Modifica : ci sarà solo un'istanza dell'oggetto che contiene questo metodo, quindi ho aggiornato l'oggetto lock in modo che non sia statico.

C'è un modo migliore per farlo? O forse questo è semplicemente sbagliato per qualsiasi motivo?

Answer 1

Sembra ragionevole se sei solo interessato a non avere il metodo eseguito in parallelo. Non c'è nulla che gli impedisca di correre immediatamente uno dopo l'altro, ad esempio se hai premuto il pulsante per mezzo microsecondo dopo che il timer ha eseguito Monitor.Exit ().

E avere anche l'oggetto lock come solo statico ha senso.

Answer 2

Puoi anche usare Mutex o Semaphore se vuoi che funzioni cross process (con una leggera penalità prestazionale), o se devi impostare un numero diverso da uno dei thread simultanei consentiti che eseguono il tuo codice.

Esistono altri costrutti di segnalazione che funzionerebbero, ma il tuo esempio sembra fare il trucco, e in modo semplice e diretto.

Answer 3

Minor nit: se la variabile lockObject è statica, allora " this.lockObject " non dovrebbe compilare. Sembra anche leggermente strano (e almeno dovrebbe essere ampiamente documentato) che sebbene questo sia un metodo di istanza, ha anche un comportamento distintamente a livello di tipo. Forse renderlo un metodo statico che utilizza un'istanza come parametro?

Utilizza effettivamente i dati dell'istanza? Altrimenti, rendilo statico. In tal caso, dovresti almeno restituire un valore booleano per dire se hai fatto il lavoro con l'istanza o meno - trovo difficile immaginare una situazione in cui voglio un po 'di lavoro con un particolare dato, ma non lo faccio attenzione se quel lavoro non viene eseguito perché un lavoro simile veniva eseguito con un diverso dato.

Penso che dovrebbe funzionare, ma sembra un po 'strano. In genere non sono un fan dell'utilizzo del blocco manuale, solo perché è così facile sbagliarsi, ma questo sembra a posto. (Devi considerare eccezioni asincrone tra il "se" e il "prova", ma sospetto che non costituiranno un problema: non ricordo esattamente le garanzie fatte dal CLR.)

Answer 4

Penso che Microsoft raccomanda utilizzando lock , anziché utilizzare direttamente la classe Monitor. Offre un layout più pulito e garantisce il rilascio del blocco in tutte le circostanze.

public class MyClass
{

  // Used as a lock context
  private readonly object myLock = new object();

  public void DoSomeWork()
  {
    lock (myLock)
    {
      // Critical code section
    }
  }
}

Se l'applicazione richiede il blocco per estendere tutte le istanze di MyClass è possibile definire il contesto del blocco come campo statico:

private static readonly object myLock = new object();

Answer 5

Il codice va bene, ma sarebbe d'accordo con la modifica del metodo in modo che sia statico poiché trasmette meglio l'intenzione. È strano che tutte le istanze di una classe abbiano un metodo tra loro che viene eseguito in modo sincrono, tuttavia quel metodo non è statico.

Ricorda che puoi sempre avere il metodo sincrono statico da proteggere o privato, lasciandolo visibile solo alle istanze della classe.

public class MyClass
{ 
    public void AccessResource()
    {
        OneAtATime(this);
    }

    private static void OneAtATime(MyClass instance) 
    { 
       if( !Monitor.TryEnter(lockObject) )
       // ...

Answer 6

Questa è una buona soluzione anche se non sono davvero contento del blocco statico. In questo momento non stai aspettando il blocco in modo da non avere problemi con i deadlock. Ma rendere i blocchi troppo visibili può facilmente metterti nei guai la prossima volta che dovrai modificare questo codice. Inoltre questa non è una soluzione molto scalabile.

Di solito provo a fare in modo che tutte le risorse che cerco di proteggere non siano accessibili da più thread di variabili di istanza private di una classe e quindi ho anche un lucchetto come variabile di istanza privata. In questo modo è possibile creare un'istanza di più oggetti se è necessario ridimensionare.

Answer 7

Un modo più dichiarativo per farlo è usare Identificatore MethodImplOptions.Synchronized sul metodo con cui si desidera sincronizzare l'accesso:

[MethodImpl(MethodImplOptions.Synchronized)] 
public void OneAtATime() { }

Tuttavia, questo metodo è sconsigliato per diversi motivi, molti dei quali possono essere trovati qui e qui . Sto pubblicando questo in modo da non sentirti tentato di usarlo. In Java, sincronizzato è una parola chiave, quindi potrebbe venire in mente durante la revisione di schemi di threading.

Answer 8

Abbiamo un requisito simile, con l'aggiunta del requisito che se il processo di lunga durata viene nuovamente richiesto, dovrebbe accodarsi per eseguire un altro ciclo dopo il completamento del ciclo corrente. È simile a questo:

https://codereview.stackexchange.com / domande / 16150 / Singleton-task-running-con-task-attesa-peer-review-sfida

private queued = false;
private running = false;
private object thislock = new object();

void Enqueue() {
    queued = true;
    while (Dequeue()) {
        try {
            // do work
        } finally {
            running = false;
        }
    }
}

bool Dequeue() {
    lock (thislock) {
        if (running || !queued) {
            return false;
        }
        else
        {
            queued = false;
            running = true;
            return true;
        }
    }
}