[PATCH] fhemweb.js longpoll

[herrmannj]

Hallo,

der aktuelle longpoll hat einen Schönheitsfehler weil der XHR responseText im Dauerbetrieb unendlich wächst.

Deshalb crashen clients die zum Beispiel mit webViewControl und Floorplan im Dauerbetrieb ohne page refresh laufen nach einigen Stunden/Tagen. Der patch behebt das.

Leider ist der responseText vom XHR read-only. Im vorgeschlagenen patch wird der XHR deswegen geschlossen und neu gestartet wenn eine Schwelle von 300k überschritten ist. Die Schwelle kann in Zeile 7 angepasst werden. Dazu komme kleine kosmetische Korrekturen. Nebenwirkungen konnte ich im Test nicht beobachten.

Code Auswählen Erweitern


3,4c3
< var FW_poll_seen; // poll bytes processed
< var FW_poll_query;
---
> var FW_curLine; // Number of the next line in FW_pollConn.responseText to parse
6,7c5
< var FW_poll_op = 0; // 0: normal op, 1: reset, 2: suspend, 3: leaving
< var FW_poll_size = 300000; //size of longpoll buffer
---
> var FW_leaving;
54,64c52
<   if(FW_pollConn.readyState == 4) {
<     if (FW_poll_op > 1) return;
<     if (FW_poll_op == 1) {
<       FW_poll_op = 0;
<       FW_poll_seen = 0;
<       FW_pollConn = new XMLHttpRequest();
<       FW_pollConn.open("GET", FW_poll_query + new Date().getTime(), true);
<       FW_pollConn.onreadystatechange = FW_doUpdate;
<       FW_pollConn.send(null);
<       return;
<     }
---
>   if(FW_pollConn.readyState == 4 && !FW_leaving) {
73,82c61,64
<   var complete = FW_pollConn.responseText.lastIndexOf("\n");
<
<   if (complete <= FW_poll_seen)
<     return;
<
<   var lines = FW_pollConn.responseText.slice(FW_poll_seen, complete).split("\n");
<   FW_poll_seen = complete + 1;
<   
<   // log("Longpoll buffer fill " + FW_pollConn.responseText.length + " bytes");
<
---
>   var lines = FW_pollConn.responseText.split("\n");
>   //Pop the last (maybe empty) line after the last "\n"
>   //We wait until it is complete, i.e. terminated by "\n"
>   lines.pop();
84c66
<   for(var i = 0; i < lines.length; i++) {
---
>   for(var i=FW_curLine; i < lines.length; i++) {
86c68
<     //log("Longpoll: " + i + " : " +(l.length>132 ? l.substring(0,132)+"...("+l.length+")":l));
---
>     log("Longpoll: "+(l.length>132 ? l.substring(0,132)+"...("+l.length+")":l));
117a100,101
>   //Next time, we continue at the next line
>   FW_curLine = lines.length;
124,128d107
<
<   if ((FW_pollConn.responseText.length > FW_poll_size) && (complete == FW_pollConn.responseText.length -1)) {
<     FW_poll_op = 1;
<     FW_pollConn.abort();
<   }
133a113
>   FW_curLine = 0;
177,179c157
<   FW_poll_seen = 0;
<   FW_poll_op = 0;
<   FW_poll_query = document.location.pathname+"?XHR=1"+
---
>   var query = document.location.pathname+"?XHR=1"+
181,182c159,160
<                 "&timestamp=";
<   FW_pollConn.open("GET", FW_poll_query + new Date().getTime(), true);
---
>                 "&timestamp="+new Date().getTime();
>   FW_pollConn.open("GET", query, true);
350,355c328,329
<     if(isiOS) {
<       // script load called before delayed longpoll started
<       if (FW_pollConn) {
<         FW_poll_op = 2;
<         FW_pollConn.abort();
<     }
---
>     if(isiOS)
>       FW_pollConn.abort();
359c333
<       if(isiOS &&  FW_poll_op == 2)
---
>       if(isiOS)
361d334
<       }
412c385
<   FW_poll_op = 3;
---
>   FW_leaving = 1;


ps.: hab ich diff so richtig verwendet ?

vg
Jörg

[herrmannj]

Hi,

vermutlich habe ich den patch vorschlag im falschen Format geliefert, das Problem und/oder Lösung unklar geschildert oder der post ist einfach untergegangen

Was kann ich tun damit der patch evtl Anwendung findet ?

vg
Jörg

[Dr. Boris Neubert]

Länger als nur einen Tag warten?

Viele Grüße
Boris

[betateilchen]

@Boris Du verlangst ja Sachen.... 

[rudolfkoenig]

@herrmannj: der Patch ist nicht untergegangen, sondern wartete auf ausreichend Freizeit am Stueck -> je komplizierter der Patch, desto laenger die Wartezeit   Diesen Patch habe ich als kompliziert (1Std+) eingestuft, weil es nicht direkt das geschilderte Problem loest, sondern die Verarbeitung umstellt. Damit brauche ich laenger, um zu verstehen, was warum so geloest wurde, und welche Nebeneffekte es hat.

Anmerkungen:
- Warum ruft FW_doUpdate nicht mehr FW_longpoll auf?
- Bei der Aenderungen bei Zeile 329  fehlt ein {}
- Wozu ist die Unterscheidung zw.  FW_poll_op = 2/3 gut?
- Wieso muss man FW_poll_op = 2 setzen? Was passiert ohne?

Grundsaetzlich habe ich aber nichts dagegen, und bin bereit ihn einzubauen.

Davon abgesehen ist der patch im falschen Format, da es nicht mit "diff -u" erstellt wurde, und deswegen nur bei einem noch unveraenderten fhemweb.js greift.

[herrmannj]

Hallo Rudi,

no prob, ich hab mir sowas schon gedacht. Ich versuche Deine Zeit von 1h+ auf weniger zu drücken.

Strukturiert:

Issue #1 longpoll:
* die XHR responseText im Client wird nie auf Null gesetzt wächst mit jeder longpoll Nachricht  -> Client crasht wegen Memory Mangel.
* Lösung: XHR von Zeit zu Zeit zurücksetzen. Für client transparent.
Issue #2 dynamisches JS laden:
* die aktuelle Lösung "scheint" synchron, ist es aber nicht.
* Lösung: nach diversen Nachforschungen (stackoverflow etc): nur ein XHR objekt (synchron) garantiert browser übergreifend die synchronität. Btw, dadurch entfallen die Browserweichen.

Detail:

* line 7 definiert ein Longpoll buffer von 300k. Wenn der responseText 300k überschreitet wird geprüft ob die letzte Nachricht vollständig empfangen wurde (responseText endet mit "\n", line 125). Wenn ja wird der XHR geschlossen und neu geöffnet. Damit ist der Speicher wieder freigegeben.

Bei dieser Gelegenheit habe ich dann gleich noch ein wenig aufgeräumt. Bisher wurde ja der XHR.responseText zuerst in das lines array übernommen und das dann geparst. Das erschien mir doppelt - die Info liegt ja im responseText sowieso vor.  "FW_poll_seen" ist ein pointer auf den char im responseText bis zu dem der responseText bereits abgearbeitet ist.  Line 73 "complete" ist ein pointer auf das jeweils letzte (am weitesten rechts) "\n" im responseText, also auf der Ende der neuesten, komplett empfangenen Longpoll Nachricht.

Der Text zwischen "FW_poll_seen" und "complete" wird also als temporäres array wie gewohnt geparst (line 78) belegt aber nur temporär Speicher (lines brauchte permanent).

Warum ruft FW_doUpdate nicht mehr FW_longpoll auf?

Indirekt schon. FW_longpoll erstellt ja die query (raum etc) und führt sie dann sofort als XHR aus.

Ich unterstelle das sich in fhemweb Instanzen die query nicht ändert während die Seite geladen ist. Deshalb wird der query String beim ersten Aufruf in line 179 in "FW_poll_query" hinterlegt.  "FW_doUpdate" muss in der Folge die query nicht neu berechnen sondern darf in line 60 den XHR direkt neu aufmachen. Wenn Du in Hinblick auf spätere Erweiterungen (websockets) Bedarf siehst das zu kapseln dann müsste man dort (line 57-63) wieder über  "FW_doUpdate" gehen. Kann man mAn dann immer noch machen 

Bei der Aenderungen bei Zeile 329  fehlt ein {}

mia culpa, das habe ich unreflektiert aus der Vorlage übernommen.  gefixt.

Wozu ist die Unterscheidung zw.  FW_poll_op = 2/3 gut?

2 steht für temporäres suspend (während des JS ladens / IOS). 3 ist das "alte" FW_leaving. Im Kern habe ich mit "3" die bestehende Funktionalität von "fw_leaving" 1:1 übernommen ohne daran rumzuschrauben und mit "2" das suspend ergänzt. Für "3": safety Gedanke, es funktioniert also hab ich das übernommen.

Wieso muss man FW_poll_op = 2 setzen? Was passiert ohne?

wie eben geschrieben. "2" ist der suspend des longpoll während des dynamischen JS ladens. Vor dem laden des js via XHR wird der longpoll geschlossen um das W3C Verbindungslimit einzuhalten (was derzeit nur von Apple beachtet wird).  Der geschlossene longpoll löst jedoch einen call auf "FW_doUpdate()" aus (readyState = 4). Der natürliche reflex von "FW_doUpdate()" wäre jetzt die Verbindung als kaputt zu betrachten und sie neu zu öffnen (was genau hier falsch wäre).

FW_poll_op = 2 sagt deshalb der "FW_doUpdate()" das der longpoll absichtlich still gelegt wurde: "Füße stillhalten" 
In line 356 wird der lonpoll (auf IOS) nach Abschluss des js ladens manuell wieder angeworfen, diesmal über FW_longpoll().

Ich erstelle den patch neu mit "-u" und hänge ihn an.

Zusätzlich folgende Bitte:

Ich habe eine usecase bei dem jquery Bedienelemente dynamisch geladen werden und bei der Instanzspezifischer code (nicht Modul!) benötigt wird.
Daher kippe ich Javascript Initialisierungen beim initialen pageload über die FW_summary des devices ein was top funktioniert und super kompatibel zu fhemweb und floorplan ist.

Anschließende longpolls fragen jetzt via FW_notify -> FW_devState -> FW_summary das device wieder ab (gut so!).

Hier wäre es extrem hilfreich wenn FW_Summary einen weiteren Parameter bekommen könnte der im Device eine Unterscheidung ermöglicht ob der longpoll fragt (dann muss ich den Initialisierungscode nicht mitschicken und kann schick Bandbreite sparen) oder ob die Anfrage vom pageLoad kommt.

Vielleicht hast Du eigene Vorstellungen zur Umsetzung, Verschlag sonst:
fhemweb:
line 2176
- my ($allSet, $cmdlist, $txt) = FW_devState($dn, "", \%extPage);
+ my ($allSet, $cmdlist, $txt) = FW_devState($dn, "", \%extPage, 1);
line 2221
- my ($d, $rf, $extPage) = @_;
+my ($d, $rf, $extPage, $longpoll) = @_;
line 2300
- my $newtxt = &{$sfn}($FW_wname, $d, $FW_room, $extPage);
+my $newtxt = &{$sfn}($FW_wname, $d, $FW_room, $extPage, $longpoll);

Das dürfte mAn kein bestehendes Modul beeinflussen und man könnte in der device FW_summary zukünftig $longpoll == true auswerten. Wenn der pagelLoad fragt dann sollte $longpoll == undef sein.

Danke und Grüße
Jörg

[herrmannj]

Hi Rudi,

Teil II: der Unterscheidung in der FW_summary ob der caller longpoll oder pageload ist.

Könntest Du mir (bitte ) eine "Wasserstandsmeldung" geben ob Du es in Betracht ziehst der FW_summary so eine Unterscheidungsmöglichkeit zu spendieren. Ich will nicht pushy sein, ich steh nur im development gerade an einer Stelle wo ich mich entscheiden muss ob ich darauf setzen kann (Kernentscheidung). 

Wann (und wie) das dann konkret kommt ist mir vorerst egal. Wenn Du möchtest teste und liefere ich auch gerne einen Patch - wie Du möchtest.

Danke und Grüße
Jörg

[rudolfkoenig]

Prinzipiell ja.
5k+ Text zu lesen (und zu verstehen) dauert vermutlich noch laenger, als eine Stunde

[herrmannj]

sorry, da bin ich teilschuldig, mittlerweile hat die Baustelle halt drei Bauabschnitte.

Damit Du nicht den Faden verlierst: das das Thema fw_summary erarbeite ich einen patchvorschlag und mache einen neuen thread auf.

Damit bleiben hier:
* longpoll memory leak
* dynamisches script load.

Danke
Jörg

"Am Ende wird alles gut, wenn es nicht gut wird, ist es noch nicht das Ende" (Oscar Wilde)

[rudolfkoenig]

Hallo Jörg,

ich habe das memory-leak Problem jetzt auf meine Weise geloest (und eingecheckt) mit mAn minimalen Aenderungen, und deswegen vermutlich auch minimalen Nebenwirkungen.

Wenn Du weitere Aenderungen haben moechtest dann bitte einzeln, und moeglichst ohne Aufraeumaktionen, sonst verliere ich den Ueberblick.

Gruss,
  Rudi

[herrmannj]

hmm,

1. der Speicherverbrauch ist doppelt so groß wie nötig weil XHR responseText und lines die bereits geparsten msgs halten.
2. wenn (300*1024) bytes erreicht sind wird der xhr resettet - völlig unabhängig davon ob die letzte msg komplett war oder nicht ...

Schöner, selten auftretender und kaum reproduzierbarer Fehler ... 

vg
jörg