Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: watch mode for run command #110

Merged
merged 1 commit into from
Oct 14, 2024
Merged

feat: watch mode for run command #110

merged 1 commit into from
Oct 14, 2024

Conversation

tumidi
Copy link
Contributor

@tumidi tumidi commented Jun 18, 2024

Hängt ab von: questionpy-org/questionpy-server#105

Hängt ab von: questionpy-org/questionpy-server#106

@tumidi
Copy link
Contributor Author

tumidi commented Jul 30, 2024

Hab den Code in d9b6c14 jetzt noch mal refaktorisiert. Die wichtigsten Änderungen:

  • beinhaltet nicht mehr Quellcode-Verzeichnisse beim Ausführen immer neu bauen #108, da mittlerweiler gemergt
  • kommt jetzt ohne feat: allow package reloading questionpy-server#105 aus
  • mehr async hier und da
    • Thread-Synchronisierung in Watcher-Klasse entfernt (weniger Fehleranfälligkeit, einfacherer Code):
      Es wird ein Callback an den watchdog-Thread durchgereicht. Der watchdog-Thread kümmert sich nur noch um das Filewatching. Die Logik für Paket neubauen, Server neustarten, etc passiert in Coroutinen im Hauptthread.
  • Der run-Befehl hat nun die Optionen --port und --host

@tumidi tumidi requested a review from larsbonczek July 30, 2024 10:00
questionpy_sdk/watcher.py Show resolved Hide resolved
questionpy_sdk/watcher.py Outdated Show resolved Hide resolved
questionpy_sdk/webserver/app.py Outdated Show resolved Hide resolved
questionpy_sdk/webserver/app.py Outdated Show resolved Hide resolved
Copy link
Member

@MHajoha MHajoha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ich glaube die Implementierung lässt sich recht stark vereinfachen:

  • Die Trennung der Logik in start_watching/__aenter__ und run_forever erlaubt zwar theoretisch, andere Dinge vor, nach oder statt des Webservers zu machen, dafür haben wir aber zumindest aktuell keinen Use-Case und ich kann mir kurzfristig keinen vorstellen. Wenn du auch keinen hast, würde ich vorschlagen, Watcher in eine Funktion (oder wenige, aber halt nicht X Mathoden) zu vereinen.
  • Ich glaube es reicht ein asyncio.Event statt einer Condition, oder?
  • Ich vermute es ist nicht nötig, den Debouncer und Observer jedes mal komplett zu stoppen und neu zu starten. Eine Flag ignore_all_events: bool würde doch ausreichen? Ansonsten bietet watchdog auch eine unschedule-Methode.

questionpy_sdk/webserver/app.py Outdated Show resolved Hide resolved
questionpy_sdk/watcher.py Outdated Show resolved Hide resolved
questionpy_sdk/watcher.py Outdated Show resolved Hide resolved
questionpy_sdk/watcher.py Outdated Show resolved Hide resolved
@tumidi
Copy link
Contributor Author

tumidi commented Aug 1, 2024

  • Die Trennung der Logik in start_watching/__aenter__ und run_forever erlaubt zwar theoretisch, andere Dinge vor, nach oder statt des Webservers zu machen, dafür haben wir aber zumindest aktuell keinen Use-Case und ich kann mir kurzfristig keinen vorstellen. Wenn du auch keinen hast, würde ich vorschlagen, Watcher in eine Funktion (oder wenige, aber halt nicht X Mathoden) zu vereinen.

Ja, siehe auch Kommentor oben.

Der Use-Case war halt, dass die Klasse grundsätzlich auch separat verwendet werden kann. Aber keine Ahnung, wie wichtig das ist. Hab's mal rausgenommen.

  • Ich glaube es reicht ein asyncio.Event statt einer Condition, oder?

Da bin ich mir nicht 100% sicher. Es könnte eine Race condition geben zwischen await self._on_change_event.wait() und self._unschedule(), da der Event Listener ja in einem eigenen Thread läuft und Events feuern kann. Die würden zwar nicht abgearbeitet werden, da der control flow der run_forever-Methode ja gerade in der while True-Schleife steckt, aber ich weiß nicht was das sonst für Seiteneffekte hat, wenn z.B. Events gepuffert werden und ähnliches.

Die Verwendung eines Locks schien mir am sichersten in dem Fall, da ausgeschlossen ist, dass einkommende Events irgendwie zum Zug kommen während gerade ein Rebuild/Server-Restart passiert.

Hab's mal mit einem asyncio.Event implementiert und konnte ad-hoc keinen Unterschied feststellen, was nicht heißt, dass es nicht doch ein Bug ist. 🤷🏽

  • Ich vermute es ist nicht nötig, den Debouncer und Observer jedes mal komplett zu stoppen und neu zu starten. Eine Flag ignore_all_events: bool würde doch ausreichen? Ansonsten bietet watchdog auch eine unschedule-Methode.

Das war vor der letzten Refaktorisierung auch so implementiert.

Die Idee dahinter war, dass wenn man builds hat die zehntausende Dateien erzeugen, verändern (npm, etc.), man während des Builds eine große Anzahl unnützer Events hat, die man dropped.

Hab es jetzt mit schedule/unschedule ersetzt, dass sollte dann keine Events erzeugen und die Threads des Debouncer/Observer werden nicht unnötig neugestartet.

Copy link
Member

@MHajoha MHajoha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Die Verwendung eines Locks schien mir am sichersten in dem Fall, da ausgeschlossen ist, dass einkommende Events irgendwie zum Zug kommen während gerade ein Rebuild/Server-Restart passiert.

Da du korrekterweise das Event erst clear()st, nachdem Rebuild und Restart durch sind, sollten seitere set()s währenddessen keinen Effekt haben.

questionpy_sdk/watcher.py Outdated Show resolved Hide resolved
@tumidi tumidi merged commit 8d7cc91 into dev Oct 14, 2024
6 checks passed
@tumidi tumidi deleted the feat/watch branch October 14, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants