Development #14606
durée de vie des processus phantomjs
100%
Description
De manière générale quand le python lance un phantomjs, après un certain temps sans réponse, il faudrait le tuer automatiquement et passer à autre chose (genre l'affichage d'un message d'indisponibilité à l'usager).
Fichiers
Demandes liées
Révisions associées
Historique
Mis à jour par Josué Kouka il y a environ 7 ans
- Lié à Bug #14173: Tuer le processus phantomjs au bout d'un certain temps sans retour ajouté
Mis à jour par Josué Kouka il y a environ 7 ans
- Statut changé de Nouveau à En cours
- Assigné à mis à Josué Kouka
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
- Patch proposed changé de Non à Oui
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
avec un test
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
suppression du print dans le test
Mis à jour par Thomas Noël il y a environ 7 ans
Mes petites remarques :
- result.get('stderr', None) : le None est inutile
- "Phantomjs process stalled with conetext :" : → "PhantomJS process timeout, context:"
- J'ai pas compris le data.pop('locators') (ie ça me semble pas prévu que la fonction modifie data) *
Mis à jour par Serghei Mihai il y a environ 7 ans
En plus data
est un dico et sa représentation chaîne n'est pas toujours la même. Chez moi c'est:
{'auth_checker': 'tenants/static/js/auth.checker.js', 'address': 'https://whatever.com', 'cookies': [], 'form_submit_element': 'input[type=submit], button'}
Je dirais qu'il faudrait virer cookies
car il peut y avoir pas mal et ça ne nous sert pas à grande chose.
Mis à jour par Serghei Mihai il y a environ 7 ans
Oublions pour l'instant la dernière phrase de mon commentaire concernant le cookies
Mis à jour par Josué Kouka il y a environ 7 ans
Thomas Noël a écrit :
Mes petites remarques :
- result.get('stderr', None) : le None est inutile
- "Phantomjs process stalled with conetext :" : → "PhantomJS process timeout, context:"
- J'ai pas compris le data.pop('locators') (ie ça me semble pas prévu que la fonction modifie data)
Le couple login/mdp(déchiffré) s'y trouve, du coup j'ai préféré ne pas l'envoyer dans les logs.
Mis à jour par Frédéric Péters il y a environ 7 ans
Le couple login/mdp(déchiffré) s'y trouve, du coup j'ai préféré ne pas l'envoyer dans les logs.
Invitation à laisser un commentaire dans le code.
Mis à jour par Thomas Noël il y a environ 7 ans
Ok, mais il ne faut pas faire un "pop" pour cela, qui modifie data (et l'appelant va se retrouver avec un "data" modifié).
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
En prennant en compte les differentes remarques
Mis à jour par Josué Kouka il y a environ 7 ans
Thomas Noël a écrit :
Le test il va attendre 10 secondes ?
Oui, mais je peux setter PHANTOM_JS_WAITING_TIME=2
dans le settings de test pour qu'on attende pas trop longtemps
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
Josué Kouka a écrit :
Thomas Noël a écrit :
Le test il va attendre 10 secondes ?
Oui, mais je peux setter
PHANTOM_JS_WAITING_TIME=2
dans le settings de test pour qu'on attende pas trop longtemps
J'ai ajouté le PHANTOMJS_WAITNG_TIME=2
dans tests/settings.py.
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-if-stalled-14606.patch 0001-kill-phantomjs-if-stalled-14606.patch ajouté
Thomas Noël a écrit :
Pourquoi 2 alors que 1 suffit :)
:D, je pense que 1/4s devrait suffir.
Mis à jour par Thomas Noël il y a environ 7 ans
Il manque le commentaire qui explique le pourquoi du « context = {k: v for k, v in data.items() if k != 'locators'} ».
Par ailleurs, juste pour être tatasse, selon moi inutile d'appeler "waiting_time" ce qui s'appelle un "timeout" (et ça sous-entend l'abandon, c'est mieux).
Et cependant, ne pas appeler ta variable "PHANTOMJS_..." car juste à côte on a déjà "PHANTOM_JS_BINARY" : il faut donc plutôt le format "PHANTOM_JS_TIMEOUT".
Et aller, pour finir, je poserais pour rappel la variable dans mandayejs/settings.py (PHANTOM_JS_TIMEOUT=10). Comme ça on sait qu'elle existe, tu peux même ajouter une petite ligne de doc pour dire à quoi elle sert.
(fallait pas m'inviter)
Mis à jour par Josué Kouka il y a environ 7 ans
Mis à jour par Thomas Noël il y a environ 7 ans
- « timeout = getattr(settings, 'PHANTOM_JS_TIMEOUT', 10) » ne sert plus à rien, utiliser directement « process.join(settings.PHANTOM_JS_TIMEOUT) »
- les petits détails font la qualité : c'est pas « Phantomjs process timeout, context : » que j'avais proposé mais « PhantomJS process timeout, context: » (ça s'écrit "PhantomJS" et on ne met pas d'espace avant : en anglais)
- ligne 67: result = {'result': 'failure'} : il faudrait préciser que c'est suite à un timeout
- « # remove user's credentials », plutôt : « don't log locators, they may contains credentials (passwords) »
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-process-if-timeout-14606.patch 0001-kill-phantomjs-process-if-timeout-14606.patch ajouté
Thomas Noël a écrit :
- « timeout = getattr(settings, 'PHANTOM_JS_TIMEOUT', 10) » ne sert plus à rien, utiliser directement « process.join(settings.PHANTOM_JS_TIMEOUT) »
- les petits détails font la qualité : c'est pas « Phantomjs process timeout, context : » que j'avais proposé mais « PhantomJS process timeout, context: » (ça s'écrit "PhantomJS" et on ne met pas d'espace avant : en anglais)
- ligne 67: result = {'result': 'failure'} : il faudrait préciser que c'est suite à un timeout
- « # remove user's credentials », plutôt : « don't log locators, they may contains credentials (passwords) »
Avec ajout d'un message expliquant à l'utilisateur que l'application n'a pas répondu dans le temp imparti.
Mis à jour par Thomas Noël il y a environ 7 ans
supprimer le "result = None" ligne 32... et ack.
Mis à jour par Josué Kouka il y a environ 7 ans
- Fichier 0001-kill-phantomjs-process-if-timeout-14606.patch 0001-kill-phantomjs-process-if-timeout-14606.patch ajouté
Thomas Noël a écrit :
supprimer le "result = None" ligne 32... et ack.
Ok
Mis à jour par Josué Kouka il y a environ 7 ans
- Statut changé de En cours à Résolu (à déployer)
- % réalisé changé de 0 à 100
Mis à jour par Frédéric Péters il y a plus de 5 ans
- Statut changé de Résolu (à déployer) à Fermé
kill phantomjs process if timeout (#14606)