Projet

Général

Profil

Development #14606

durée de vie des processus phantomjs

Ajouté par Frédéric Péters il y a environ 7 ans. Mis à jour il y a plus de 5 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Josué Kouka
Catégorie:
-
Version cible:
-
Début:
13 janvier 2017
Echéance:
% réalisé:

100%

Temps estimé:
Patch proposed:
Oui
Planning:

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

Lié à Mandaye - Bug #14173: Tuer le processus phantomjs au bout d'un certain temps sans retourFermé30 novembre 2016

Actions

Révisions associées

Révision ecb03255 (diff)
Ajouté par Josué Kouka il y a environ 7 ans

kill phantomjs process if timeout (#14606)

Historique

#1

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é
#2

Mis à jour par Josué Kouka il y a environ 7 ans

  • Statut changé de Nouveau à En cours
  • Assigné à mis à Josué Kouka
#3

Mis à jour par Josué Kouka il y a environ 7 ans

#4

Mis à jour par Josué Kouka il y a environ 7 ans

avec un test

#5

Mis à jour par Josué Kouka il y a environ 7 ans

suppression du print dans le test

#6

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) *
#7

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.

#8

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

#9

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.

#10

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.

#11

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é).

#12

Mis à jour par Josué Kouka il y a environ 7 ans

En prennant en compte les differentes remarques

#13

Mis à jour par Thomas Noël il y a environ 7 ans

Le test il va attendre 10 secondes ?

#14

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

#15

Mis à jour par Josué Kouka il y a environ 7 ans

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.

#16

Mis à jour par Thomas Noël il y a environ 7 ans

Pourquoi 2 alors que 1 suffit :)

#17

Mis à jour par Josué Kouka il y a environ 7 ans

Thomas Noël a écrit :

Pourquoi 2 alors que 1 suffit :)

:D, je pense que 1/4s devrait suffir.

#18

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)

#20

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) »
#21

Mis à jour par Josué Kouka il y a environ 7 ans

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.

#22

Mis à jour par Thomas Noël il y a environ 7 ans

supprimer le "result = None" ligne 32... et ack.

#23

Mis à jour par Josué Kouka il y a environ 7 ans

Thomas Noël a écrit :

supprimer le "result = None" ligne 32... et ack.

Ok

#24

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
#25

Mis à jour par Frédéric Péters il y a plus de 5 ans

  • Statut changé de Résolu (à déployer) à Fermé

Formats disponibles : Atom PDF