Project

General

Profile

Anomalie #4345

super_cron HS en https

Added by jluc - 4 months ago. Updated 3 months ago.

Status:
En cours
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
06/11/2019
Due date:
% Done:

0%

Resolution:
Navigateur:

Description

action_super_cron_dist appelle fsockopen avec le port 80 par défaut, si l'url issue de genererl_url_action ne spécifie pas le port (c'est à dire la plupart du temps).

sur un site en https ça échoue (éventuellement ça renvoie une 301 mais fsockopen ne les suit pas)

Faute de parvenir à proposer une PR git, voici un patch à l'ancienne qui corrige ça en testant si http ou https.

En plus, il loge les échecs de fsockopen afin de pas avoir une partie silencieuse de SPIP qui ne marche pas.
Et il teste aussi le statut renvoyé. Cette partie peut être supprimée si nécessaire.

supercron.diff View (1.18 KB) jluc -, 06/11/2019 01:05 PM

supercron_simple.diff View (1002 Bytes) jluc -, 06/11/2019 01:12 PM

supercron v2.diff View (1.41 KB) jluc -, 06/11/2019 05:23 PM

History

#1 Updated by jluc - 4 months ago

voici le diff dont la partie qui teste le statut du fsockopen a été retirée - au cas où vous préféreriez

#2 Updated by jluc - 4 months ago

Sauf si c'est évident le dev qui regarde, c'est à vérifier avant commit.

#3 Updated by jluc - 4 months ago

C'est pas fini, il faut aussi ajouter ssl:// à l'host

#4 Updated by jluc - 4 months ago

Je confirme 443 + ssl:// avant le host comme argument de fsockopen, mais pas dans le GET : comme ça le super_cron marche en https

#5 Updated by jluc - 4 months ago

Et j'ai du ajouter un t=time() ou t=intval(time()/10) dans la query aussi, pour que l'url appelée ne soit pas interpelée par le varnish...

#6 Updated by jluc - 4 months ago

Voici le nouveau .diff avec tout : "supercron v2"
- le port 443 et le préfixe ssl:// pour fsockopen si on en est https
- les logs d'échecs de connexion et de fsockopen
- le timestamp sur l'url pour bypasser les caches

#7 Updated by jluc - 4 months ago

Pour vraiment tester le retour du fsockopen on peut aussi ajouter :

$response = fgets($fp, 79);
$statut_init = substr($response, 9, 1);
if ($statut_init != '2')
   spip_log("Erreur statut renvoyé pour $url statut renvoyé = $response...", 'erreur_super_cron'.LOG_ERREUR);

#8 Updated by jluc - 4 months ago

Et puis tant qu'à faire, changer les commentaires :

 * Cette fonction se termine tout de suite, après avoir lancée un cron asynchrone
 * Elle est inadaptée depuis un cron UNIX dans lesquels il faut directement appeler l'action cron

#9 Updated by b b 4 months ago

  • Status changed from Nouveau to En cours

Le patch grossit à vue d'oeil, pourquoi ajouter une nouvelle constante _SUPER_CRON_DELAIS ?

De plus, il serait intéressant de s'inspirer de ce que fait https://core.spip.net/projects/spip/repository/entry/spip/ecrire/inc/queue.php#L630 pour la détection du port.

Et pour finir, ça serait pas mal de respecter les PSRs dans tes structures de test :)

#10 Updated by jluc - 4 months ago

Il y a en effet plusieurs trucs : une correction de code, une amélioration potentielle et des modifs des commentaires.

La constante _SUPER_CRON_DELAIS permet d'ajouter un timestamp sur l'appel de cron. Cela vise à ce que le php reçoive vraiment la requête au lieu qu'elle soit interceptée par un varnish "mal configuré" (comme les timestamp qu'on ajoute aux fichiers images)
Dans le cas où il y a un cache de ce type et si la valeur de _SUPER_CRON_DELAIS vaut plus que 1, cela divise d'autant la fréquence des appels au cron en cas de grosse fréquentation.
Si ça convient pas comme ça, on pourrait la garder mais ne pas diviser le timestamp avec (et alors, la renommer _SUPER_CRON_TIMESTAMP )

Autre point :
il faudrait utiliser les constantes _PORT_HTTP_STANDARD et _PORT_HTTPS_STANDARD si définies plutôt que 80 et 443,
ici et aussi ailleurs : dans la fonction queue_affichage_cron

#11 Updated by jluc - 4 months ago

Pour produire un diff à intégrer directement il me faudrait savoir ce que je met dedans : tout ? Sinon c'est en kit :
- le fix pour 80 / 443 via les constantes et en testant le scheme comme queue_affichage_cron ? (a priori oui !)
- l'emploi d'un timestamp ?
- le timestamp divisé qui introduit un délai ?
- le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8
- les logs dans les divers cas d'erreur déjà testé ?
- le test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX ?
- la correction de queue_affichage_cron pour employer les constantes ?

#12 Updated by jluc - 3 months ago

Autrement présentés, le kit des possibles, c'est :

Correction des bugs sans améliorations supplémentaires :
- le fix pour le port 80 / 443 via les constantes
- ceci en testant le scheme comme queue_affichage_cron
- le remplacement des commentaires phpdoc comme indiqué plus haut ? https://core.spip.net/issues/4345#note-8

Améliorations de la qualité du code, sans changement fonctionnel :
- loger des divers cas d'erreurs testés
- la correction de queue_affichage_cron pour employer les constantes

Améliorations fonctionnelles :
- test du statut de retour du fsockopen en plus, avec log si c'est pas 2XX
- emploi d'un timestamp
- timestamp divisé qui introduit un délai

Also available in: Atom PDF