eine Funktion könnte laut Code Maintainer blockieren

Blaubart

Ich habe einem Treiber für ein Anzeigegerät weiterentwickelt und in dem Projekt einen PR erstellt.
Hier mal der Link zu der von mir erweiterten Datei:
https://github.com/XCSoar/XCSoar/blob/618f6523dea388d4adf8e608fd56653e27e6e3fd/src/Device/Driver/AirControlDisplay.cpp
Es geht um die Funktion OnCalculatedUpdate ab Zeile 205.
Der Maintainer kritisiert aber, dass PortWriteNMEA() blockieren könnte:

PortWriteNMEA() can block, and you circumvented the API protection by using a NullOperationEnvironment, just to make it compile, but without knowing the implications. Don't do random stuff just to make it compile!

In der Funktion darüber, OnSensorUpdate, wird es aber genauso bereits eingesetzt. Außerdem in einem anderen Treiber: https://github.com/XCSoar/XCSoar/blob/master/src/Device/Driver/CaiLNav.cpp z.B. Zeile 126.

Ich verstehe nicht, was genau kritisiert wird, bzw. warum es mal so OK ist, in meinem Fall aber nicht. Der Maintainer ist nicht gerade hilfsbereit mit seinen Kommentaren, und ihn brauche ich leider nicht zu fragen.

Vielleicht kann jemand Licht ins Dunkel bringen?

Also A ist deine gewünschte Änderung unformatiert (schon allein das ist Grund genug, den PR abzuweisen).

Und B sagt dir der Maintainer schon eine ganze Menge:

https://github.com/XCSoar/XCSoar/pull/1185

This function can block. It must not be called from within "OnCalculatedUpdate()", because the latter must never block.

Diese Funktion kann blockieren. Sie darf nicht aus "OnCalculatedUpdate()" heraus aufgerufen werden, da letztere niemals blockieren darf.

this isn't the line I commented on

how is sprintf supposed to block?

"PortWriteNMEA()" can block, and you circumvented the API protection by using a "NullOperationEnvironment", just to make it compile, but without knowing the implications. Don't do random stuff just to make it compile! If you don't understand your own code, it's bad code.

"PortWriteNMEA()" kann blockieren, und Sie haben den API-Schutz umgangen, indem Sie ein "NullOperationEnvironment" verwendet haben, nur um es zu kompilieren, ohne die Folgen zu kennen. Machen Sie keine willkürlichen Dinge, nur damit es kompilierbar ist! Wenn Sie Ihren eigenen Code nicht verstehen, ist es schlechter Code.

Und damit hat der Maintainer vollkommen recht. Er sagt dir, was falsch ist und erklärt dir warum.

Auch hier im Forum gilt: Bei der Wahrheit bleiben, das hilft meist am meisten.

DocShoe

@Fragender sagte in eine Funktion könnte laut Code Maintainer blockieren:

Auch hier im Forum gilt: Bei der Wahrheit bleiben, das hilft meist am meisten.

Alter... komm mal runter und mach mal langsam mit deinem Kreuzzug.

@DocShoe Ja, schon gut... Aber das ärgert mich, immer sind die anderen schuld und nie man selber...

Leon0402

Er hat doch genau die Stelle sogar zitiert @Fragender hier:

@Blaubart sagte in eine Funktion könnte laut Code Maintainer blockieren:

Es geht um die Funktion OnCalculatedUpdate ab Zeile 205.
Der Maintainer kritisiert aber, dass PortWriteNMEA() blockieren könnte:
PortWriteNMEA() can block, and you circumvented the API protection by using a NullOperationEnvironment, just to make it compile, but without knowing the implications. Don't do random stuff just to make it compile!
In der Funktion darüber, OnSensorUpdate, wird es aber genauso bereits eingesetzt. Außerdem in einem anderen Treiber: https://github.com/XCSoar/XCSoar/blob/master/src/Device/Driver/CaiLNav.cpp z.B. Zeile 126.
Ich verstehe nicht, was genau kritisiert wird, bzw. warum es mal so OK ist, in meinem Fall aber nicht.

Zur Aussage über den Maintainer. Joa, also er schreibt schon Sachen, aber freundlich wirkte der Ton des Maintainers jetzt auch nicht auf mich, wenn ich ehrlich bin. Also ich kann die Kritik schon nachvollziehen. Klar kann man auch seine Änderungen formattieren etc. und inhaltlich ist das auch berechtigte Kritik, aber die kann man auch netter verpacken, wenn man neue Leute nicht vergraulen möchte.

Aber gut das ist ein Thema alles für sich. Die Maintainer machen es ja auch alle nur in ihrer Hobby und das die vlt. genervt sind, wenn sie auf so Basic Sachen aufmerksam machen muss, kann man schon auch irgendwie verstehem. Also ich kann da beide Seiten ein wenig nachvollziehen

Quiche Lorraine

@Blaubart

Ich habe mir den Code mal angeschaut und zurückverfolgt:

Die Funktion ACDDevice::OnSensorUpdate() sieht ok aus.
Auch die Funktion PortWriteNMEA() sieht erst einmal unauffällig aus, also habe ich mir mal port.FullWrite() angeschaut.
Die Funktion Port::FullWrite() sieht auch erst einmal unauffällig aus, ruft aber die Funktion Write() auf. Ich vermute hier könnte SerialPort eine Spezialisierung von Port sein.
Also habe ich einen Blick in SerialPort::Write() geworfen und eine verdächtige Stelle gefunden; sWriter.Wait();.
In OverlappedEvent.hpp fand ich die folgende Implementierung:

WaitResult Wait(unsigned timeout_ms=INFINITE) {
    switch (::WaitForSingleObject(os.hEvent, timeout_ms)) {
    // ...
  }
};

Und das blockiert definitiv, wenn das Event os.hEvent nicht eintritt.

@Leon0402 Nein, es geht nicht um einen freundlichen Umgangston. Das Review war angemessen. Bei uns geht es teils noch sehr viel rauer zu ... Es kommt auf den Inhalt an und nicht auf die Verpackung, oder auf @Blaubart s Wohlbefinden. Letztendlich darf kein "falscher" Code in production stage oder master einfließen. Linus Torvalds "reißt auch regelmäßig die Hutschnur" ... und Elon Musk hat ja fast schon ein psychopathisches Verhalten ...

Quellen:
https://www.heise.de/news/Linus-Torvalds-explodiert-Manche-Sicherheitsleute-sind-verfickte-Idioten-3895116.html

Leon0402

@Fragender sagte in eine Funktion könnte laut Code Maintainer blockieren:

@Leon0402 Nein, es geht nicht um einen freundlichen Umgangston. Das Review war angemessen. Bei uns geht es teils noch sehr viel rauer zu ... Es kommt auf den Inhalt an und nicht auf die Verpackung, oder auf @Blaubart s Wohlbefinden. Letztendlich darf kein "falscher" Code in production stage oder master einfließen. Linus Torvalds "reißt auch regelmäßig die Hutschnur" ... und Elon Musk hat ja fast schon ein psychopathisches Verhalten ...

Quellen:
https://www.heise.de/news/Linus-Torvalds-explodiert-Manche-Sicherheitsleute-sind-verfickte-Idioten-3895116.html

Ja und das Verhalten, welches Elon Musk und Linus Torvalds an den Tag legen ist auch unter aller Sau manchmal. Selbstverständlich geht es um einen freundlichen Umgangston und nicht nur um den Inhalt. Das es bei euch teils sehr viel rauer zu geht, tut mir sehr leid für dich. Aber deswegen muss man das ja nicht gut finden Es gibt Dinge, die können Leute tun, weil sie die entsprechende Macht / Geld etc. haben, so ist es leider. Eine Entschuldigung für schlechtes Benehmen ist das trotzdem nicht.
Und es fließt kein falscher Code in Produktion, nur weil man seine inhaltliche korrekte Kritik freundlich rübergebracht hat. Das eine hat mit dem anderen gar nichts zu tun. Er soll ja nicht durchwinken beim Review. Das ist nicht, was ich mit freundlich sein meine.

Aber gut jeder hat die Wahl am Ende Es ist das Recht von jedem unfreundlich zu sein. Aber dann darf man sich auch nicht beschweren, wenn keiner mit einem arbeiten will. Umgekehrt muss man auch nicht super empfindlich sein. Ich kenne es auch durchaus nach dem Motto "Nicht geschimpft ist genug gelobt". Am Ende ist ein Zwischending optimal imo.

Ich hab ja auch nichts gegen ein gelegentliches Lob. Das ist manchmal sogar ein Muss (damit keiner wegläuft ...). Aber man kann halt nicht einfach schreiben, das hast du toll gemacht, wenn dabei eine technische oder funktionale Anforderung einfach umgangen wurde ... (hier in diesem Fall der fehlende Einsatz von Parallelität)

Aber ich glaube () summa summarum, wir beide meinen inhaltlich schon dasselbe.

Blaubart

Erst mal danke, dass ihr euch das angeschaut habt!! Nun mir geht es wirklich nicht um mein Wohlbefinden oder darum funktionale Anforderungen zu umgehen! Ich will es richtig machen!! Abgesehen davon möchte ich hier definitiv niemanden in ein schlechtes Licht stellen und möchte mich daher mit Ausführungen über den Maintainer zurückhalten. Das wäre wenn sowie so nur meine Meinung und muss so nicht von anderen gesehen werden!! Außerdem gehört es hier nicht hin. Nur so viel: Ich bin mit vielen im Kontakt, die mit der Art wie er kommentiert ihre Probleme habe, weil es oft nicht weiterhilft. Aber bitte belassen wir es dabei!

Es ist mir vollkommen klar, das kein schlampiger Code in den Master aufgenommen werden soll! Ich will es ja auch verbessern, habe aber noch keine Idee wie. Und außerdem verwirrt es mich dann, dass in der gleichen Datei ähnlicher und in einer anderen Datei fast der gleiche Code eingesetzt wird. Mal vorausgesetzt der Maintainer hat hier keinen Mist gebaut, muss es ja etwas geben, was bei mir anders und daher problematisch ist. Das war eigentlich meine erste Frage.

@Quiche-Lorraine hat ja schon herausgefunden, warum es zur Blockade kommen kann. Meine zweite Frage, wie kann ich das nun umgehen? Mit umgehen meine ich nicht mit schlampigem Code Sondern mit korrekter Programmierung

Blaubart

hmmm, ich nehme an, meine Frage ist so einfach nicht zu beantworten, oder?

Quiche Lorraine

@Blaubart sagte in eine Funktion könnte laut Code Maintainer blockieren:

hmmm, ich nehme an, meine Frage ist so einfach nicht zu beantworten, oder?

Das ist der Punkt.

Meiner Meinung nach müsste man hier eine Code-Analyse machen. Detailiert nachschauen wo überall der Code blockieren kann, was hierbei alles passieren kann, ob hier evt. schon eine Lösungsmöglichkeit im XCSoar Projekt exisitiert,...

Und wo ich gerade die Funktion OnCalculatedUpdate() sehe, würde ich mal prüfen wie oft die Funktion aufgerufen wird. Nicht dass irgentwelche Threads die Funktion gleichzeitig aufrufen, wie mir das neulich mit meiner OnNewVideoImage() Funktion passiert ist.

Eine schnelle und saubere Lösungsmöglichkeit sehe ich keine.

Blaubart

OK, trotzdem danke für die Unterstützung.