2010-12-07 15 views
1

Ich implementiere diese Klasse als Singleton. Ich bin nicht gut in der Fadensicherheit. Wollte sicherstellen, dass die GenerateOrderID-Klasse Thread-sicher ist. Genauer gesagt, die orderCount-Variable kann nicht gleichzeitig von verschiedenen Objekten inkrementiert werden und den Countdown auslösen.Ist diese Klasse Thread sicher?

Antwort

8

Es ist nicht. Die Post-Inkrement-Operation ist nicht atomar. Sie folgende Änderungen vornehmen müssen:

Ersetzen Sie die GenerateOrderID Methode mit diesem:

public string GenerateOrderID() 
{ 
    return String.Format("{0:yyyyMMddHHmmss}{1}", 
         DateTime.Now, 
         Interlocked.Increment(ref orderCount)); 
} 

und initialisieren orderCount-0 anstelle von 1, da Interlocked.Increment den erhöhten Wert zurückgibt. (Mit anderen Worten, ist Interlocked.Increment(ref foo) identisch in jeder Hinsicht ++foo außer dass es atomar, und deshalb Thread-sicher.)

Beachten Sie, dass Interlocked.Increment ist viel effizienter als die Verwendung von lock Threads zu synchronisieren, obwohl lock noch Arbeit. Siehe this question.

Verwenden Sie auch keine Singletons.

+0

Vielen Dank für Ihre ausführliche Antwort. Nur neugierig, warum nicht Singles, besonders in diesem Fall? – bkarj

+0

@Behrooz: Weil Singletons effektiv die Flexibilität aus gutem Grund begrenzen. Geben Sie vor, dass Sie Ihren Dienst/Ihre Anwendung erweitert haben, um Bestellnummern für zwei Datenbanken zu verwalten. Ein Singleton würde dies unmöglich machen. Sie sollten stattdessen die Abhängigkeitsinjektion oder ein "Kontext" -Objekt verwenden, das herumgereicht wird und auf eine "OrderIDGenerator" -Instanz verweist. – cdhowie

1

Ich würde OrderCount als flüchtig markieren (um Compiler Optimierungsannahmen zu verhindern) und eine Sperre um das Inkrement verwenden. Das volatile ist wahrscheinlich zu viel, aber es tut nicht weh.

public class OrderIDGenerator 
{ 

    private static readonly OrderIDGenerator instance = new OrderIDGenerator(); 
    private volatile int orderCount; 
    private static object syncRoot = new object(); 


    private OrderIDGenerator() 
    { 
     orderCount = 1; 
    } 


    public static OrderIDGenerator Instance 
    { 
     get { return instance; } 
    } 

    public string GenerateOrderID() 
    { 
     lock (syncRoot) 
      return String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++); 
    } 
} 
+1

Sperren ist auch Overkill; 'Interlocked.Increment' ist für dieses Szenario besser geeignet. – cdhowie

+0

Ach ja, natürlich. Hinterlasse meine ursprüngliche Antwort für die Nachwelt (aber geh mit cdhowies Antwort oben). –

0

Sie haben es einfach

public class OrderIDGenerator { 
    private static readonly OrderIDGenerator instance = new OrderIDGenerator(); 
    private int orderCount; 
    private static readonly object _locker = new object(); 

    private OrderIDGenerator() { 
      orderCount = 1; 
    } 

    public static OrderIDGenerator Instance { 
      get { return instance; } 
    } 

    public string GenerateOrderID() { 
     string orderId = ""; 
     lock (_locker) { 
      orderID = String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++); 
     } 
     return orderID; 
    } 
} 
3

Diese Klasse ist nicht thread mit einer kleinen Modifikation Thread sicher zu sein.

Inkrement ist keine atomare Operation. Es ist daher durchaus möglich, dass ein Kontextschalter gerade zur falschen Zeit passiert:

Start with orderCount at 1. 
Two threads try to GenerateOrderID() at once: 

Thread 1    | Thread 2 
---------------------------------------------- 
read orderCount = 1 | 
        --> 
        | read orderCount = 1 
        | add: 1 + 1 
        | write orderCount = 2 
        <-- 
add: 1 + 1   | 
write orderCount = 2 | 
return timestamp1 | 
        --> 
        | return timestamp1 

Sie jetzt einen doppelten Auftrag ID mit Ihrer Bestellung ab Zählung geworfen hat.

Um dies zu beheben, sperrt Ordercount, während er den Zugriff auf:

public string GenerateOrderID() 
{ 
    lock(this){ 
     return String.Format("{0:yyyyMMddHHmmss}{1}", DateTime.Now, orderCount++); 
    } 
} 

Das „Schloss“ Anweisung nur einen nach dem anderen Thread in kann für jedes Objekt auf erfaßt werden kann. Wenn sich ein Thread in GenerateOrderID() befindet, wird Ihr OrderIDGenerator bis zum Abschluss gesperrt, und der nächste Thread, der versucht, die Sperre zu beanspruchen, muss warten, bis der erste Thread vollständig ausgeführt wurde.

Lesen Sie mehr über die lock Anweisung here.

+0

Als Anmerkung: CDHowie hat Recht.Interlocked.Increment (ref orderCount) ist besser als die gesamte lock-Anweisung. –

+0

+1 sehr saubere Erklärung des Threads un-Sicherheitsproblem! Glückwunsch! – Lorenzo