Discussion:
Thread-safe singleton
(zu alt für eine Antwort)
Christian H. Kuhn
2017-09-06 16:29:33 UTC
Permalink
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
Patrick Roemer
2017-09-06 19:06:25 UTC
Permalink
Post by Christian H. Kuhn
private static volatile Connection connection;
[...]
Post by Christian H. Kuhn
static Connection getConnection() throws QFdsbException {
if (null == connection) {
synchronized (connection) {
if (null == connection) {
Wenn connection null ist, synchronisierst Du darüber? O_o
Post by Christian H. Kuhn
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?
Post by Christian H. Kuhn
Andere Frage: Wird die Connection nicht am Ende schon durch den
impliziten finally-Block geschlossen, bevor sie ausgeliefert wird?
Ja.

Viele Grüße,
Patrick
Volker Borchert
2017-09-06 20:02:04 UTC
Permalink
Post by Christian H. Kuhn
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 <***@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert <***@despammed.com>
Patrick Roemer
2017-09-06 21:27:05 UTC
Permalink
Post by 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
Volker Borchert
2017-09-07 16:45:28 UTC
Permalink
Post by Patrick Roemer
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 <***@ncc1701.starfleet.fed>
"I'm a mechanic, not a doctor." Volker Borchert <***@despammed.com>
Christian H. Kuhn
2017-09-10 23:08:32 UTC
Permalink
Post by Patrick Roemer
Post by 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.
Post by Patrick Roemer
Post by Volker Borchert
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?
Post by Patrick Roemer
Der Code ist nicht klassisch kaputt sondern sehr individualistisch fritte.
Danke :-)
Post by Patrick Roemer
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
Patrick Roemer
2017-09-11 16:14:45 UTC
Permalink
Post by 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...
Post by Christian H. Kuhn
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

Marcel Mueller
2017-09-06 21:26:33 UTC
Permalink
Post by Christian H. Kuhn
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.
Post by Christian H. Kuhn
PMD widerspricht mir.
Wer auch immer das ist.
Post by Christian H. Kuhn
final class QFdsbDatabase {
private static volatile Connection connection;
Eine statische Connection, hmm ...
Post by Christian H. Kuhn
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.
Post by Christian H. Kuhn
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.
Post by Christian H. Kuhn
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.
Post by Christian H. Kuhn
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.
Post by Christian H. Kuhn
Mit
einem intitialization-on-demand holder vertage ich das Problem in eine
Unterklasse.
?
Post by Christian H. Kuhn
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
Claus Reibenstein
2017-09-10 11:52:19 UTC
Permalink
Post by Marcel Mueller
Post by Christian H. Kuhn
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
Lesen Sie weiter auf narkive:
Loading...