Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > de.comp.lang.java > #13127 > unrolled thread

Thread-safe singleton

Back to article view | Back to de.comp.lang.java

Contents

  Thread-safe singleton "Christian H. Kuhn" <qno-news@qno.de> - 2017-09-06 18:29 +0200
    Re: Thread-safe singleton Patrick Roemer <sangamon@netcologne.de> - 2017-09-06 21:06 +0200
    Re: Thread-safe singleton v_borchert@despammed.com (Volker Borchert) - 2017-09-06 20:02 +0000
      Re: Thread-safe singleton Patrick Roemer <sangamon@netcologne.de> - 2017-09-06 23:27 +0200
        Re: Thread-safe singleton v_borchert@despammed.com (Volker Borchert) - 2017-09-07 16:45 +0000
        Re: Thread-safe singleton "Christian H. Kuhn" <qno-news@qno.de> - 2017-09-11 01:08 +0200
          Re: Thread-safe singleton Patrick Roemer <sangamon@netcologne.de> - 2017-09-11 18:14 +0200
    Re: Thread-safe singleton Marcel Mueller <news.5.maazl@spamgourmet.org> - 2017-09-06 23:26 +0200
      Re: Thread-safe singleton Claus Reibenstein <4spamersonly@kabelmail.de> - 2017-09-10 13:52 +0200

#13127 — Thread-safe singleton

Hallo Gemeinde,

Ich dachte, ich hätte das mit der Thread-Sicherheit inzwischen
verstanden. PMD widerspricht mir.

final class QFdsbDatabase {

    private static volatile Connection connection;

    private static final String DATABASE = "database";

    /**
     * Private constructor to avoid instantiation.
     */
    private QFdsbDatabase() {
        // empty
    }

    /**
     * Creates a Connection to a FIDE/DSB database and stores it in a
static field for use. Access data is given by an
     * ini4j ini file.
     *
     * @return Connection to a FIDE/DSB database.
     * @throws QFdsbException
     *             Exception
     */
    static Connection getConnection() throws QFdsbException {

        if (null == connection) {
            synchronized (connection) {
                if (null == connection) {

		[... Daten aus ini4j-File, dataSource erstellen ... ]

                    try (Connection myConnection =
dataSource.getConnection()) {
                        myConnection.setAutoCommit(true);
                        connection = myConnection;
                    } catch (SQLException e) {
                        throw new QFdsbException("Problems while opening
or closing JDBC Connection to player database",
                                e);
                    }
                }
            }
        }
        return connection;
    }

}

Lazy initialization der statischen Variablen, weil ich die geworfenen
Exceptions sehen will, sonst hätte ich einen static block genommen. Mit
einem intitialization-on-demand holder vertage ich das Problem in eine
Unterklasse. Double-checked locking sollte funktionieren, PMD behauptet
das Gegenteil.

Andere Frage: Wird die Connection nicht am Ende schon durch den
impliziten finally-Block geschlossen, bevor sie ausgeliefert wird?

TIA
QNo

[toc] | [next] | [standalone]

#13129

Responding to Christian H. Kuhn:
>     private static volatile Connection connection;
[...]
>     static Connection getConnection() throws QFdsbException {
> 
>         if (null == connection) {
>             synchronized (connection) {
>                 if (null == connection) {

Wenn connection null ist, synchronisierst Du darüber? O_o

>                     try (Connection myConnection =
> dataSource.getConnection()) {
>                         myConnection.setAutoCommit(true);
>                         connection = myConnection;
>                     } catch (SQLException e) {
>                         throw new QFdsbException("Problems while opening
> or closing JDBC Connection to player database",
>                                 e);
>                     }
>                 }
>             }
>         }
>         return connection;
>     }
> 
> Lazy initialization der statischen Variablen, weil ich die geworfenen
> Exceptions sehen will, sonst hätte ich einen static block genommen. Mit> einem intitialization-on-demand holder vertage ich das Problem in eine
> Unterklasse. Double-checked locking sollte funktionieren, PMD behauptet
> das Gegenteil.

Das da oben funktioniert sicher nicht. Vergiss coole
Pseudo-Optimierungen wie DCL einfach mal. Warum willst Du überhaupt eine
Connection irgendwo statisch rumhängen lassen?

> Andere Frage: Wird die Connection nicht am Ende schon durch den
> impliziten finally-Block geschlossen, bevor sie ausgeliefert wird?

Ja.

Viele Grüße,
Patrick

[toc] | [prev] | [next] | [standalone]

#13130

Christian H. Kuhn wrote:

> Ich dachte, ich hätte das mit der Thread-Sicherheit inzwischen
> verstanden. PMD widerspricht mir.

Klassisches kaputtes Double Checked Locking. Tante Gurgel sollte das
genauso klassische Paper dazu (von IIRC Doug Lea oder William Pugh)
zutage fördern. Kaufe oder leihe Dir "Effective Java" und lies im
einschlägigen Kapitel nach, wie man Singletons richtig macht.

-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#13132

Responding to Volker Borchert:
> Klassisches kaputtes Double Checked Locking. Tante Gurgel sollte das
> genauso klassische Paper dazu (von IIRC Doug Lea oder William Pugh)
> zutage fördern. Kaufe oder leihe Dir "Effective Java" und lies im
> einschlägigen Kapitel nach, wie man Singletons richtig macht.

Der Code ist nicht klassisch kaputt sondern sehr individualistisch fritte.

Klassisches DCL ist mit dem "neuen" Memory Model und volatile nicht mehr
kaputt - was im klassischen Paper auch frühzeitig nachgetragen wurde.
Trotzdem dürften Situationen, in denen es sinnvoll ist und nachweislich
etwas bringt, eher rar sein. Das hier ist mal ziemlich sicher keine.

Und zuletzt sollte man hier nicht unbedingt nachschlagen, wie man
Singletons "richtig" macht, sondern zunächst mal kontemplieren, ob man
überhaupt eins will. Meine Vermutung: Nö.

Viele Grüße,
Patrick

[toc] | [prev] | [next] | [standalone]

#13133

Patrick Roemer wrote:

> Klassisches DCL ist mit dem "neuen" Memory Model und volatile nicht mehr
> kaputt

Ups, das volatile hab ich glatt übersehen. Aber genau mit dem "neuen"
Memory Model ist volatile auch teurer geworden.

-- 

"I'm a doctor, not a mechanic." Dr Leonard McCoy <mccoy@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert  <v_borchert@despammed.com>

[toc] | [prev] | [next] | [standalone]

#13137

Am 06.09.2017 um 23:27 schrieb Patrick Roemer:
> Responding to Volker Borchert:
>> Klassisches kaputtes Double Checked Locking. Tante Gurgel sollte das
>> genauso klassische Paper dazu (von IIRC Doug Lea oder William Pugh)
>> zutage fördern. 

> Klassisches DCL ist mit dem "neuen" Memory Model und volatile nicht mehr
> kaputt - was im klassischen Paper auch frühzeitig nachgetragen wurde.

Ich nehme an, ihr sprecht von
https://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html.

>> Kaufe oder leihe Dir "Effective Java" und lies im
>> einschlägigen Kapitel nach, wie man Singletons richtig macht.

Werde ich. Ist es das von Joshua Bloch?

> Der Code ist nicht klassisch kaputt sondern sehr individualistisch fritte.

Danke :-)

> Und zuletzt sollte man hier nicht unbedingt nachschlagen, wie man
> Singletons "richtig" macht, sondern zunächst mal kontemplieren, ob man
> überhaupt eins will. Meine Vermutung: Nö.

Bin mir immer noch nicht sicher. Plan ist inzwischen folgender:

Es gibt eine Klasse, die die eigentliche Datenbank repräsentiert, eine,
die weiß, in welchem Format die Daten aus dem Internet ankommen, und
eine, die das Format kennt, in dem die Daten aus der Datenbank
geschrieben werden sollen. Dazu eine main-Methode; ob die in einer
eigenen Klasse ist oder an eine der anderen Klassen angehängt wird, ist
vermutlich unerheblich. Aber es ist eine Aufgabe, die nicht speziell an
eine der anderen Klassen gekoppelt ist, also bekommt sie erstmal ihre
eigene Klasse.

main() erzeugt (inzwischen) ein Objekt der Datenbankklasse. main() liest
die Zugangsdaten aus einer ini4j-Datei und übergibt sie im Konstruktor.
Die Importer- und Exporter-Klassen brauchen nach aktuellem Stand der
Dinge kein Objekt; es reicht aus, wenn die import- bzw. export-Methode
statisch ist. main() übergibt das Datenbankobjekt, damit die beiden
anderen wissen, an wen sie ihre geparsten Daten zu übergeben haben.

Die Idee war, dass das Datenbankobjekt genau einmal eine Connection
herstellt. Weitere Methoden erzeugen das gerade passende
(Prepared)Statement. Der Importer führt verschiedene for-each-Schleifen
aus, in der jeweils eine Zeile des Input-Files geparst wird und die
passende Methode mit dem Datensatz als Argument ausgeführt wird. Der
Exporter holt Datensatz für Datensatz ab, solange noch Daten existieren.
Am Ende von main() wird dann ein close() aufgerufen, das
Connection.close() ausführt. Es geht um je eine halbe Million
Datensätze, da wäre der Aufwand, für jeden Datensatz eine neue
Connection und ein neues Statement zu erstellen und wieder zu schließen,
ETWAS groß.

Nachdem ich sicher sein kann, dass immer nur genau eine Datenbank
gleichzeitig geöffnet ist, war die erste Idee, die Connection als
statisches Singleton anzulegen. Ich habe gelernt, dass die Idee
zumindest falsch umgesetzt war. Die Thread-Sicherheit war ein Symptom,
dass etwas nicht stimmt, auch wenn die geplante Anwendung
single-threaded ist und Multithreading nicht geplant ist. Jedenfalls im
Moment nicht, aber das könnte sich jederzeit ändern.

Bleibt die Frage, wie ich das, was ich will, richtig umsetze ...

lg
QNo

[toc] | [prev] | [next] | [standalone]

#13138

Responding to Christian H. Kuhn:
> Es gibt eine Klasse, die die eigentliche Datenbank repräsentiert, eine,
> die weiß, in welchem Format die Daten aus dem Internet ankommen, und
> eine, die das Format kennt, in dem die Daten aus der Datenbank
> geschrieben werden sollen. Dazu eine main-Methode; ob die in einer
> eigenen Klasse ist oder an eine der anderen Klassen angehängt wird, ist
> vermutlich unerheblich. Aber es ist eine Aufgabe, die nicht speziell an
> eine der anderen Klassen gekoppelt ist, also bekommt sie erstmal ihre
> eigene Klasse.
> 
> main() erzeugt (inzwischen) ein Objekt der Datenbankklasse. main() liest
> die Zugangsdaten aus einer ini4j-Datei und übergibt sie im Konstruktor.
> Die Importer- und Exporter-Klassen brauchen nach aktuellem Stand der
> Dinge kein Objekt; es reicht aus, wenn die import- bzw. export-Methode
> statisch ist. main() übergibt das Datenbankobjekt, damit die beiden
> anderen wissen, an wen sie ihre geparsten Daten zu übergeben haben.

Es gibt in Java keine Sonderpunkte, wenn man möglichst viel statisch
macht. Eher im Gegenteil...

> Die Idee war, dass das Datenbankobjekt genau einmal eine Connection
> herstellt. Weitere Methoden erzeugen das gerade passende
> (Prepared)Statement. Der Importer führt verschiedene for-each-Schleifen
> aus, in der jeweils eine Zeile des Input-Files geparst wird und die
> passende Methode mit dem Datensatz als Argument ausgeführt wird. Der
> Exporter holt Datensatz für Datensatz ab, solange noch Daten existieren.
> Am Ende von main() wird dann ein close() aufgerufen, das
> Connection.close() ausführt. Es geht um je eine halbe Million
> Datensätze, da wäre der Aufwand, für jeden Datensatz eine neue
> Connection und ein neues Statement zu erstellen und wieder zu schließen,
> ETWAS groß.

DataSource dataSource = createDataSource();
try(DB db = new DB(dataSource)) {
  new Importer(db).importData(sourceDir());
  new Exporter(db).exportData(targetDir());
}
catch(SQLException exc) {
  // ...
}

...?

Viele Grüße,
Patrick

[toc] | [prev] | [next] | [standalone]

#13131

On 06.09.17 18.29, Christian H. Kuhn wrote:
> Ich dachte, ich hätte das mit der Thread-Sicherheit inzwischen
> verstanden.

Oh, das ist ein weites Areal. Es würde mich wundern, wenn es überhaupt 
jemanden gibt, der das bis zum Ende verstanden hat.

> PMD widerspricht mir.

Wer auch immer das ist.


> final class QFdsbDatabase {
>
>      private static volatile Connection connection;

Eine statische Connection, hmm ...

>      static Connection getConnection() throws QFdsbException {
>
>          if (null == connection) {
>              synchronized (connection) {

Das wird nix. Das hatten wir ja schon.
Es darf natürlich nur *ein* Objekt geben, auf das synchronisiert wird. 
Und null ist ganz schlecht.

>                  if (null == connection) {
>
> 		[... Daten aus ini4j-File, dataSource erstellen ... ]
>
>                      try (Connection myConnection =
> dataSource.getConnection()) {

Das wird auch nichts, es sei denn es sollen geschlossene Verbindungen 
geliefert werden. Die lokale Variable im try muss weg.

>                          myConnection.setAutoCommit(true);
>                          connection = myConnection;
>                      } catch (SQLException e) {
>                          throw new QFdsbException("Problems while opening
> or closing JDBC Connection to player database",
>                                  e);
>                      }

Die Exception wird hier immer nur an einen Aufrufer ausgeliefert. Die 
anderen öffnen eine neue Verbindung - serialisiert, wenn man das Double 
Check richtig implementiert. Das kann bei systematischen Fehlern 
ziemlich langsam werden bzw. in eine DOS-Attacke ausarten.

Besser die SQLException speichern und immer wieder ausliefern.
Dabei muss man aber darauf achten, dass die null Bedingung jetzt 
entweder auf die Exception oder auf die Connection greifen muss. 
Alternativ nimmt man Typ Object und packt mal das eine und mal das 
andere rein. Lässt es sich nicht auf Connection casten, muss man halt 
throw machen.

> Lazy initialization der statischen Variablen, weil ich die geworfenen
> Exceptions sehen will, sonst hätte ich einen static block genommen.

Das ginge mit eben genannter Semantik (Exception speichern) trotzdem.


> Mit
> einem intitialization-on-demand holder vertage ich das Problem in eine
> Unterklasse.

?

> Double-checked locking sollte funktionieren, PMD behauptet
> das Gegenteil.

Implemetierungsfehler.


Es gibt aber noch eine Untiefe:
die gelieferte Connection ist mit an Sicherheit grenzender 
Wahrscheinlichkeit nicht thread-safe. Damit ist die Funktion 
getConnection in unsynchronisiertem Kontext praktisch unbrauchbar. Und 
selbst synchronisiert müsste nicht nur getConnection synchronisiert 
werden, sondern auch alle darüber laufenden Zugriffe.
Wenn aber alles rund um die Benutzung der Connection ohnehin 
synchronisiert sein muss, dann braucht man das ganze 
Double-Check-Gefuddel auch nicht mehr es wird dann ohnehin serialisiert 
gearbeitet.

Wenn hingegen umgekehrt mehrere Threads parallel die DB nutzen sollen, 
braucht man mehrere Connections und /kein/ Singleton. Das bedeutet jeder 
Thread muss die mit getConnection gelieferte Verbindung mit try/finally 
anfordern und freigeben.

Wenn jetzt wiederum nicht gewünscht ist, dass dabei jedes mal eine 
/neue/ Verbindung aufgemacht wird, braucht man eine Connection pro 
Thread. Das könnte man noch mit Thread Local Storage und Lazy Init lösen 
(unsynchronisiert).

Wenn darüber hinaus gewünscht ist, dass sich mehrere Threads Connections 
teilen, soweit sie nicht gleichzeitig eine Verbindung brauchen, dann 
braucht man einen Connection Pool. Dabei ändert sich die Semantik des 
von getConnection gelieferten Objektes so, dass es bei Close() nicht 
etwa geschlossen wird, sondern schlicht zurück in den Pool geht.
Der Zugriff auf den Pool muss natürlich synchronisiert werden. Das 
bedeutet, getConnection benötigt in jedem Fall ein synchronized. Damit 
ist der Double Check ebenfalls hinfällig.

Kurzum, es gibt kein denkbares Szenario, in dem Double-Check hier Sinn 
ergibt.


Marcel

[toc] | [prev] | [next] | [standalone]

#13135

Marcel Mueller schrieb am 06.09.2017 um 23:26:

> On 06.09.17 18.29, Christian H. Kuhn wrote:
> 
>> PMD widerspricht mir.
> 
> Wer auch immer das ist.

Ich tippe auf <https://de.wikipedia.org/wiki/PMD_%28Software%29> (erster
Treffer bei Google).

Gruß
Claus

[toc] | [prev] | [standalone]

Back to top | Article view | de.comp.lang.java

csiph-web