search

contao / core

Contao 3 → see contao/contao for Contao 4
GNU Lesser General Public License v3.0
490 stars 214 forks source link

C3.0.4: Problem mit Cache Dateien bei gleichzeitigem schreiben und lesen #5307

Closed BugBuster1701 closed 11 years ago

BugBuster1701 commented 11 years ago

Mal angenommen der Cache ist leer. (cron, frische Installation, ...) Nun folgen 2 Zugriffe zu selben Zeit. Der erste bewirkt, dass z.b. die Cache Datei dca/tl_module.php geschrieben wird. Während der geschrieben wird, fängt der zweite schon an die zu lesen, da die Datei ja bereits existiert. Nun ist das lesen aber schneller als das schreiben und die Sache explodiert. 

PHP Warning:  Unterminated comment starting line ...
PHP Parse error:  syntax error, unexpected '*' in ....
PHP Parse error:  syntax error, unexpected ''load'=>'eager' ...

Ich habe mal mit 100 Requests, davon max 10 gleichzeitig, gestestet, davon sind 14 auf Fehler gelaufen.

Das ist keine Theorie. Getestet habe ich das, da es in der Praxis bereits aufgetreten ist und ich die Ursache, die als Theorie vorlag, prüfen wollte.

BugBuster1701 commented 11 years ago

Sogar nur 10 Requests sehe ich grad, die aber wie beschrieben gleichzeitig. Die 14 Fehler sind Warnings und Error zusammen, daher mehr als 10. :-)

leofeyer commented 11 years ago

Ich hatte doch für Contao 3 das Framework so umgebaut, dass Dateien immer zuerst ins temporäre Verzeichnis geschrieben und nach Fertigstellung in einer atomaren Operation an den Bestimmungsort verschoben werden. Aber irgendwie ist der Code noch immer derselbe wie in Contao 2.11 :(

leofeyer commented 11 years ago

Hab die eigentlich vorgesehene Implementierung in c75e2a9818541b851e38b8492250baf4cea56f79 hinzugefügt (eigener Branch). Das klärt auch, wieso es überhaupt zu dem Problem in #5259 kommen konnte.

leofeyer commented 11 years ago

Jetzt gilt es nur noch abzuwägen, ob wir diese Änderung im Rahmen eines Bugfix-Release veröffentlichen, da sich dadurch die API ändert.

@contao/workgroup-core

leo-unglaub commented 11 years ago

@leofeyer Warum soll sich dadurch die API ändern? Der Erweiterung an sich es ist ja relativ wurscht wo und wie das File gecached wird. Eine Erweiterung benötigt lediglich die Funktionalität, dass die Werte aus den Cache Files geladen werden. Mehr ist nicht notwendig. Daher ist das auch ohne einen API Bruch machbar.

Du musst ja nicht das Caching-Verfahren an sich ändern (auch wenn ich das prinzipiell begrüßen würde) du musst lediglich den Core so umbauen, dass nicht Prozessübergreifend geschrieben wird. Das ist über einen Mutex locker machbar. Es muss ja nicht sein, dass der Prozess deines Webseitenbesuchers die Cache files schreibt. Das kann ja der Cron erledigen der NIEMALS gleitgleich laufen kann.

Dadurch wäre das Problem schnell gelöst.

leofeyer commented 11 years ago

Schau Dir den Code noch mal an:

Deswegen gibt es jetzt auch eine Methode exists().

Für mich ist das eine klare API-Änderung, denn new File() verhielt sich bisher wie touch() und wer das so verwendet hat, muss seinen Code anpassen.

psi-4ward commented 11 years ago

Für mich war new File() == touch() schon immer in Bug!

leo-unglaub commented 11 years ago

Moment, ich glaube wir reden hier von 2 verschiedenen Dingen. Es macht keinen Sinn das aktuelle Caching verfahren gleich zu belassen und nur in die File() Klasse einen Mutex/Lock einzubauen. Das wäre ja wieder nur eine Behebung des Symtoms und nicht des zu grunde liegenden Problems.

Wenn ich nur die File() Klasse locke baue ich mir ein riesen Performence Problem ein, da dann Webseitenbesucher=PHP Prozesse teilweise warten müssen. Das ist defakto nicht annehmbar wenn man mal mehr als 100 Leute gleichzeitig auf einer Seite hat.

Ich wäre wie oben bereits geschrieben dafür, dass der normale Contao Seitenaufruf den Cache nie verändern kann. Sondern nur lesen. Der Cronjob von Contao soll die Caches befüllen. Dadurch kannst du nie in eine Situation rennen wo das Problem hier akut wird. :)

BugBuster1701 commented 11 years ago

Aber der Cache muss immer aktuell sein, stell dir vor du installierst eine Erweiterung. Wie muss ich mir das dann vorstellen, erst mal im Cache suchen (DCA) und wenn da nicht fündig dann im Modulverzeichnis? Oder geht das jetzt bereits so?

leo-unglaub commented 11 years ago

@BugBuster1701: Ich glaube es gibt hier ein generelles Missverständnis über das Verhalten von einem Cache. Ein Cache ist ein "Ding" welches "komplizierte und rechenintensive "Dinge" zwischenspeichert, damit diese nicht jedes mal erneut berechnet werden müssen. Ein Cache ist jedoch nicht ein Ort auf dessen Existens und Datenaktuallität ich mich verlassen darf.

Das klingt jetzt sehr geschwollen, vereinfacht gesagt ist ein Cache ein Zwischenspeicher aus dem man einen Wert beziehen kann falls er dort liegt, wenn nicht muss ich ihn mir neu errechnen. Das heißt ja, dass ich mich nicht drauf verlassen kann das ich ein Sprachfile gecached habe. Wenn es nicht da ist, dann muss ich mir die Daten "on the fly" berechnen. Dass dies Stink-Langsam ist, okay. Aber wenn man den Cache deaktiviert will man ja diese aktuallität/neuberechnung der Daten.

leofeyer commented 11 years ago

und nur in die File() Klasse einen Mutex/Lock einzubauen

Ich hatte die besagte Implementierung gewählt, weil sie genau ohne Locks auskommt. Locks machen mehr Probleme, als sie beheben.

leo-unglaub commented 11 years ago

Locks machen mehr Probleme, als sie beheben.

Hmm, das möchte ich nicht so einfach unterschreiben. Locks funktionieren wunderbar wenn man sie an den richtigen Stellen einsetzt. Zum Beispiel verwende ich das Databank-Tabellen locking täglich während meiner Arbeit und hatte noch nie Probleme damit. Auch bei meiner täglichen Arbeit mit C Programmen habe ich dabei nie Probleme gehabt.

Aber das nur zum generellen Statement. Was die Contao implementierung angeht schrieb ich ja extra folgendes:

Wenn ich nur die File() Klasse locke baue ich mir ein riesen Performence Problem ein, da dann Webseitenbesucher=PHP Prozesse teilweise warten müssen. Das ist defakto nicht annehmbar wenn man mal mehr als 100 Leute gleichzeitig auf einer Seite hat.

Um noch mal meinen Beitrag von oben zu zitieren, ich wäre für eine solche Lösung:

Ich wäre wie oben bereits geschrieben dafür, dass der normale Contao Seitenaufruf den Cache nie verändern kann. Sondern nur lesen. Der Cronjob von Contao soll die Caches befüllen. Dadurch kannst du nie in eine Situation rennen wo das Problem hier akut wird. :)

Damit wäre dieses Problem ein für alle mal behoben und das ohne Api änderung.

leofeyer commented 11 years ago

Ist klar. Und wie oft soll so ein Cronjob ausgeführt werden? Selbst wenn er minütlich laufen würde, wären das 59 Sekunden, in denen alle Anfragen ohne Cache laufen müssen. Und wenn sich danach mal was ändert, dauert es wieder 59 Sekunden, bis die Änderungen in den Cache geschrieben wurden.

tristanlins commented 11 years ago

Und wenn sich danach mal was ändert, dauert es wieder 59 Sekunden, bis die Änderungen in den Cache geschrieben wurden.

Aber ist es nicht so, dass Änderungen sowieso erst nach Ablauf der Cache Time zu einem Neu-Generieren führen? (Außer wenn man von Hand den Cache löscht)

Diese Diskussion war irgendwann schon mal, da wurde unter anderem ein Intelligentes Caching als Lösung vorgeschlagen, dies wurde auch abgelehnt. Ein primitives oder dummes Caching wie es aktuell gemacht wird, führt nun mal zu diesen Problemen :-(

leofeyer commented 11 years ago

Ich habe eine Idee, wie man die Änderung rückwärtskompatibel machen könnte. Problematisch ist ja nur folgender Aufruf:

// Create an empty new file
new File('path/to/file');

Man könnte daher einfach im Destruktor prüfen, ob mit dem Objekt irgendeine Aktion ausgeführt wurde. Falls nicht, wird wie früher die Datei erstellt, wodurch die das Objekt wieder wie touch() verhält.

leo-unglaub commented 11 years ago

Ich finde jede Minute ist viel zu hoch. Ich würde das einmal pro Stunde oder sogar nur einmal am Tag machen lassen. Auf einer Live-Seite ändert sich ja defakto nichts an dem Cache. Während der Entwicklung ist es deaktiviert.

Wenn man jedoch wirklich etwas am Cache ändert, dann muss man den halt so wie den Such-Index auch wieder manuel aufbauen. Ansonsten ist das einfach bis zum nächsten Cronjob leer. Das liegt dann aber in der Hand des benutzers.

leofeyer commented 11 years ago

Aber ist es nicht so, dass Änderungen sowieso erst nach Ablauf der Cache Time zu einem Neu-Generieren führen?

Nimm an, Du klickst auf "internen Cache leeren" in der Systemwartung. Beim nächsten Seitenaufruf wird der Cache neu geschrieben. Müssten wir jetzt auf einen Cronjob warten, könnte das bis zu 59 Sekunden dauern.

leo-unglaub commented 11 years ago

Nimm an, Du klickst auf "internen Cache leeren" in der Systemwartung. Beim nächsten Seitenaufruf wird der Cache neu geschrieben. Müssten wir jetzt auf einen Cronjob warten, könnte das bis zu 59 Sekunden dauern.

Ja, aber genau das beinhaltet ja der "wipe" eines caches. Wenn du bei einem T3 den Cache ganz leerst dauert das Stunden bis der wieder komplett gefüllt ist.

Was deine Lösung mit dem Destructor angeht, BITTE NICHT. Das wäre wieder ein Herumoperieren um das Problem, jedoch keine Behebung. Damit würden wir nur noch mehr "magic" einbauen die nach ein paar wochen nur noch für Verwirrung sorgt.

Ich finde die einfachste Lösung ist hier die Beste. Wenn nichts im Cache ist, ist nichts da. Punkt. Dann muss contao es halt dynamisch aufbauen. Ist ja nichts dabei. Wenn man den cache leert soll man ihn halt via Backend auch wieder aufbauen können. Aber niemals "aufbauen während des requestes".

tristanlins commented 11 years ago

Nimm an, Du klickst auf "internen Cache leeren" in der Systemwartung.

Ist das was anderes als "(Außer wenn man von Hand den Cache löscht)" ??? verwirrtbin

Nun, was hindert Contao daran, im Anschluss an "internen Cache leeren" direkt den Cache neu aufzubauen?

tristanlins commented 11 years ago

Aber niemals "aufbauen während des requestes".

sign

leofeyer commented 11 years ago

Ok, Du hast Deinen Punkt klar gemacht. Hilft uns aber jetzt nicht, denn das komplette Austauschen der Cache-Implementierung ist frühestens in Contao 4 eine Option. Was wir jetzt brauchen, ist eine schnelle Lösung, die funktioniert und keine API-Änderungen notwendig macht. Und da erscheint mir ein bisschen "magic", den wir in Contao 4 wieder entfernen können, durchaus hilfreich.

leofeyer commented 11 years ago

Ganz abgesehen davon, geht es in diesem Ticket gar nicht nur um den Cache! Es geht darum, wie die Files-Klasse arbeitet und dass Dateien erstellt werden, bevor diese fertig geschrieben wurden.

leo-unglaub commented 11 years ago

Hilft uns aber jetzt nicht, denn das komplette Austauschen der Cache-Implementierung ist frühestens in Contao 4 eine Option.

Davon rede ich nicht. Was spricht denn dagegen einfach den Cache neu aufzubauen nachdem man die Systemwartung ausgeführt hat? Das hat nichts mit einer Neuimplementieurng des Caches zu tun.

leofeyer commented 11 years ago

Kann irgendjemand, der nicht auf Biegen und Brechen nur seine eigenen Vorstellungen durchdrücken möchte, bitte mal den Code aus b6d3bd526305545b611a36cb2e532af68b9b1ebc testen?

leo-unglaub commented 11 years ago

Ich teste es, auch wenn du mich mit deinem Kommentar wohl von den Testern ausgeschlossen hast. :(

leofeyer commented 11 years ago

Danke :)

tristanlins commented 11 years ago

Ohne es zu testen, kann ich dir sagen das es zu Problemen führt. Denn wer new File() als Ersatz für touch() missbraucht, der geht davon aus, dass die Datei auch sofort da ist. Das Problem: Du kannst nicht garantieren, dass der Destruktor auch "bei Zeiten" aufgerufen wird. In der Regel wird der Destruktor erst bei Skriptende aufgerufen!

Folgendes wird z.B. generell fehlschlagen:

$file = new File('not/existing/file');
$content = $file->getContents(); // -> file_get_contents wird einen Fehler werfen

new File() ist so gestaltet, das man sich darauf verlassen kann das die Datei vorhanden ist und nicht einfach nur ein touch() Ersatz. Das Problem ist also weitreichender und lässt sich vermutlich nicht 100% lösen. Selbst wenn man die File Methoden entsprechend weiter anpasst.

leo-unglaub commented 11 years ago

Tristan hat recht, es funktioniert im Core, jedoch nicht bei Fremd Erweiterungen. Denn, nicht jede Erweiterung macht ein unset(); auf auf das FileObject oder ruft destruct auf. Daher kann dies ebenfalls zu Problemen führen.

leofeyer commented 11 years ago

Das ist ein sehr konstruiertes Beispiel, denn was für einen Sinn hat denn bitte

$file = new File('not/existing/file');
$content = $file->getContents(); // die Datei ist immer leer!

nicht jede Erweiterung macht ein unset(); auf auf das FileObject oder ruft destruct auf

Das ist auch gar nicht notwendig, der Destruktor wird automatisch am Skriptende aufgerufen.

leo-unglaub commented 11 years ago

Ja, der wird am Script-Ende ausgeführt. Aber wenn ich 2 Objekte habe auf dem Teil habe ich doch wieder eine "race condition" und damit exakt da gleiche Problem wie BugBuster im eröffnungspost?

tristanlins commented 11 years ago

Das ist ein sehr konstruiertes Beispiel, denn was für einen Sinn hat denn bitte

Nein, das ist nicht konstruiert, sondern Real Live von mir selbst. Lass es mich so schreiben, dann ist es verständlicher wieso es das Problem gibt:

$file = new File('maybe/existing/file');
$content = $file->getContents();

Ein $file->exists() war nie notwendig, weil new File() garantiert hat, dass die Datei existiert, ergo habe ich auch keine Prüfung eingebaut. Und maybe/existing/file kann ja z.B. eine Datei sein, die eine andere Klasse "möglicherweise" schon erzeugt hat oder halt auch nicht.

leofeyer commented 11 years ago

Das Beispiel ist nach wie vor konstruiert, denn entweder schreibst Du etwas in Deine Datei, oder Du musst den Inhalt nicht abfragen, weil die Datei sowieso leer ist.

Das einzige Argument ist, dass die Datei vorher schon beim Aufruf erstellt wurde und jetzt erst am Skriptende.

tristanlins commented 11 years ago

Das einzige Argument ist, dass die Datei vorher schon beim Aufruf erstellt wurde und jetzt erst am Skriptende.

Genau darauf habe ich mich verlassen, das unter bestimmten Voraussetzungen und auch nur dann eine Datei vorher im Skriptablauf erstellt wurde, die ich dann zu einem viel späteren Zeitpunkt lese. Problem: auch wenn es "viel" später ist, kannst du nicht garantieren, dass der Destruktor aufgerufen wurde.

leofeyer commented 11 years ago

Wenn "viel später" nicht innerhalb desselben Requests ist, wurde der Destruktor ganz sicher aufgerufen (nämlich automatisch am Skriptende).

leofeyer commented 11 years ago

Ok, ich sehe schon. Dann brauchen wir halt eine neue FileAdvanced()-Klasse und ich ändere alle Aufrufe im Core. Das ist dann ganz sicher rückwärtskompatibel :)

aschempp commented 11 years ago

Bevor jemand eine FileAdvanced Klasse baut, gibt es bestimmt schon ein Package für Filemanipulation dass man nutzen könnte? Symphony anyone? ;-)

leo-unglaub commented 11 years ago

Eine FileAdvanced Klasse nur weil du nicht die 5 Zeilen änderung übernehmen willst die ich vorgeschlagen habe? Irgend wie erinnert mich das ein etwas ....

der nicht auf Biegen und Brechen nur seine eigenen Vorstellungen durchdrücken möchte

^^ ;)

tristanlins commented 11 years ago

Falls jemand ein wenig Analyse betreiben will: http://www.paste.to/NTQyMDc= Bei Bedarf kann ich auch einen größeren "Kontext" liefern :-)

tristanlins commented 11 years ago

Das ist übrigens das Ergebnis eines grep 'new File' . -C 3 -R auf meinen lokalen "ER" mirror mit allen Sources :-)

leofeyer commented 11 years ago

weil du nicht die 5 Zeilen änderung übernehmen willst

Welche 5 Zeilen Änderung?

leo-unglaub commented 11 years ago

Noch einmal: Das besagte Problem ist defakto nur bei den Cache Dateien akut. Wenn du einfach die Generierung der Cache Files in die Systemwartung verschiebst bist du fertig. Du musst lediglich ins BE eine Checkbox hinzufügen die den Neuaufbau des Caches triggert. Fertig. Das sind +- 5 Zeilen Code.

leofeyer commented 11 years ago

Ja, nur funktioniert es halt nicht. Contao schreibt an vielen Stellen Cache-Dateien, die kann man unmöglich alle in der Systemwartung vorhersehen. Ganz abgesehen davon – und das habe ich auch schon oben geschrieben – ist dieses Problem nicht auf den Cache beschränkt!

leo-unglaub commented 11 years ago

Hast du irgend ein Beispiel wo es was anderes auser den Cache betrifft?

leofeyer commented 11 years ago

Behoben in 86f53d4702e8c4feb075afdf13d8c98f1c4b6283.

tristanlins commented 11 years ago

Sieht gut aus, scheint zu laufen, jetzt sollte nur @BugBuster1701 seine Tests nochmal laufen lassen :-)

leofeyer commented 11 years ago

@tristanlins Es gab übrigens wirklich einen Fall im Core, der die File-Klasse wie touch() verwendet hat.

try {
    $file = new File('system/tmp/test');
    $file->close();
    $file->delete();
} catch (Exception $e) {
    echo 'Das temporäre Verzeichnis ist nicht beschreibbar';
}

:)

backbone87 commented 11 years ago

wo ist egtl das problem nen einfaches locking umzusetzen?

prüfe ob die gecachten dateien da sind

cache routine: versuche ein flock auf "system/tmp/system.lock"

BugBuster1701 commented 11 years ago

Also meine Tests haben nun kein Problem mehr erzeugt, sieht also gut aus. :+1:

leofeyer commented 11 years ago

Können/sollen wir noch mal kurz über Leo's Vorschlag bezüglich der Cache-Generierung via Cronjob sprechen?

Selbstverständlich – wie schon mehrfach gesagt – wäre das wenn nur für den internen Cache möglich. Die HTML-Seiten alle einmal aufzurufen halte ich für übertrieben (obwohl es noch teilweise möglich wäre) und dynamische Inhalte wie Suchergebnisse lassen sich sowieso nicht vorhersehen und automatisiert erstellen.

Möglich wären die Sprachdateien, die DCA-Dateien und die DCA-Extrakte.

Die Frage ist, wie sich die Applikation verhalten soll, wenn diese Daten noch nicht vorliegen? Der interne Cache ist ja kein optionales "Kann", sondern ein "Muss", damit z.B. die Models funktionieren. Ich weiß nicht, ob es viel bringt, wenn man zusätzliche Routinen einfügt, die dafür sorgen, dass die Daten auch ohne den internen Cache irgendwie verfügbar gemacht werden, nur damit dieser optional wird?

tristanlins commented 11 years ago

Der interne Cache ist ja kein optionales "Kann", sondern ein "Muss"

Um welche Daten geht es da genau, die ohne Cache nicht zur Verfügung stehen? Ich kenne die "neuen" Caching Mechanismen noch nicht so gut.