[spip-dev] Erreur de logique dans pipeline()

rastapopoulos · Juillet 21, 2014, 5:17

Actuellement on a :

  static $charger;

  // chargement initial des fonctions mises en cache, ou generation du cache
  if (!$charger) {
    if (!($ok = @is_readable($charger = _CACHE_PIPELINES))) {
      include_spip('inc/plugin');
      // generer les fichiers php precompiles
      // de chargement des plugins et des pipelines
      actualise_plugins_actifs();
      if (!($ok = @is_readable($charger)))
        spip_log("fichier $charger pas cree");
    }

    if ($ok) {
      include_once $charger;
    }
  }

Sauf que si le but est bien de ne pas lancer la recompilation du fichier "charger_pipelines.php" s'il a déjà été chargé, *ça ne marche pas*.

En effet, la variable $charger peut ensuite parfaitement être déjà définie SANS que la création du fichier ait marché ! (= sans que is_readable($charger) renvoie TRUE).

C'est donc le $ok qu'il faut mettre en static et tester, et non pas $charger.

Je propose donc de remplacer par ça :

  static $ok;

  // chargement initial des fonctions mises en cache, ou generation du cache
  if (!$ok) {
    if (!($ok = @is_readable($charger = _CACHE_PIPELINES))) {
      include_spip('inc/plugin');
      // generer les fichiers php precompiles
      // de chargement des plugins et des pipelines
      actualise_plugins_actifs();
      if (!($ok = @is_readable($charger)))
        spip_log("fichier $charger pas cree");
    }

    if ($ok) {
      include_once $charger;
    }
  }

En faisant cette modification, je n'ai plus aucun problème dans mes pérégrinations à installer SPIP en ligne de commande. Plus rien ne plante nulle part (pour l'instant en tout cas).

Go go go ? Ou quelqu'un a une contre-indication ?

cerdic · Juillet 21, 2014, 6:04

En fait le but c'est de charger le fichier et de le recomposer si besoin à la première occurrence d'appel. Si la reconstruction à échoué au premier coup on ne recommence pas plusieurs fois, ce sera pour un prochain hit, celui ci étant d'ores et déjà moisi.
On peut faire évoluer comme tu proposes, au moins en version dev, le temps d'être sur qu'il n'y a pas de scénarios ou les tentatives multiples feront timeout.

Cédric

rastapopoulos · Juillet 21, 2014, 8:56

Je n'ai pas remarqué de moisissure quand on appelle plusieurs fois justement.

Ce que j'ai constaté, avec mes tests d'installations en ligne de commande, c'est que parfois dans un même hit
- le premier appel le fichier est mal créé ou pas les bons droits, en tout cas is_readable() retourne FALSE
- au second appel du même pipeline, là le fichier est lisible, ça retourne TRUE et le pipeline fonctionne… pour peu que ce ne soit pas bloqué à cause du static.

J'ai remarqué ça en faisant des logs uniquement lorsque $action=='declarer_tables_principales'. Lors de l'installation de plugins ça pouvait planter (chez moi c'est Médias qui plante toujours sur creer_base_types_doc()) à cause de ça.

En effet, malgré le fait qu'au deuxième appel le fichier est lisible, c'est trop tard si on saute le test à cause du static déjà là.

Pour l'instant, effectivement : je suis obligé de relancer mon "spip installer" deux fois (deux hits donc) pour que ça marche. Ce qui est assez laid.

cerdic · Juillet 22, 2014, 8:38

Hello,

ce qui est possible c'est qu'il y ait une réentrance ou quelque chose du genre lors de la création du fichier qui explique ce problème : genre la creation initiale du fichier declenche un appel de pipeline, qui déclenche la creation initiale du fichier qui est interrompue quelque part sur un test de réentrance, du coup on sort de la fonction sans pouvoir executer le pipeline, mais au coup d'après c'est bon parce qu'on était justement en train de le construire. Un debug_backtrace() au premier appel devrait te permettre de confirmer d'où l'on vient.

Je crois qu'il ne faut pas se contenter de l'explication "le fichier est mal créé ou pas les bons droits" car ça me parait peu vraisemblable (mal créé ça peut arriver exceptionnellement sur de la concurrence, pas les bons droits ça ne peut pas arriver).

Après ton patch est peut-être une solution qui ne pose pas de problème, ou a contrario, il faut peut-être être plus prudent et regarder le backtrace si on a pas reussi a creer le fichier, et si l'on voit que c'est à cause de la pile d'appel, vider le $charger pour ressayer au prochain coup.

rastapopoulos · Juillet 22, 2014, 12:44

ce qui est possible c'est qu'il y ait une réentrance ou quelque chose du
genre lors de la création du fichier qui explique ce problème : genre la
creation initiale du fichier declenche un appel de pipeline, qui
déclenche la creation initiale du fichier qui est interrompue quelque
part sur un test de réentrance, du coup on sort de la fonction sans
pouvoir executer le pipeline, mais au coup d'après c'est bon parce qu'on
était justement en train de le construire. Un debug_backtrace() au
premier appel devrait te permettre de confirmer d'où l'on vient.

En laissant le code d'origine, à l'intérieur du `if (!$charger)` (donc seulement une seule fois), j'ai ajouté :

if ($action == 'declarer_tables_principales') {
spip_log($ok, 'debug.'._LOG_ERREUR);
spip_log(debug_backtrace(), 'debug.'._LOG_ERREUR);
}

J'obtiens alors cette trace ("charger_pipelines" n'est donc PAS chargé) :

false

Puis :

array (
  0 =>
  array (
    'file' => '/home/vincent/public_html/test/ecrire/base/objets.php',
    'line' => 332,
    'function' => 'pipeline',
    'args' =>
    array (
      0 => 'declarer_tables_principales',
      1 =>
      array (
        'spip_jobs' =>
        array (
          'field' =>
          array (
            'id_job' => 'bigint(21) NOT NULL',
            'descriptif' => 'text DEFAULT \'\' NOT NULL',
            'fonction' => 'varchar(255) NOT NULL',
            'args' => 'longblob DEFAULT \'\' NOT NULL',
            'md5args' => 'char(32) NOT NULL default \'\'',
            'inclure' => 'varchar(255) NOT NULL',
            'priorite' => 'smallint(6) NOT NULL default 0',
            'date' => 'datetime DEFAULT \'0000-00-00 00:00:00\' NOT NULL',
            'status' => 'tinyint NOT NULL default 1',
          ),
          'key' =>
          array (
            'PRIMARY KEY' => 'id_job',
            'KEY date' => 'date',
            'KEY status' => 'status',
          ),
        ),
      ),
    ),
  ),
  1 =>
  array (
    'file' => '/home/vincent/public_html/test/ecrire/base/objets.php',
    'line' => 723,
    'function' => 'lister_tables_objets_sql',
    'args' =>
    array (
    ),
  ),
  2 =>
  array (
    'file' => '/home/vincent/public_html/test/ecrire/base/create.php',
    'line' => 134,
    'function' => 'lister_tables_principales',
    'args' =>
    array (
    ),
  ),
  3 =>
  array (
    'file' => '/home/vincent/public_html/test/ecrire/install/etape_3.php',
    'line' => 92,
    'function' => 'creer_base',
    'args' =>
    array (
      0 => 'sqlite3',
    ),
  ),
  4 =>
  array (
    'file' => '/home/vincent/travail/svn/spip-zone/_outils_/spip-cli/trunk/spip-cli/CoreInstaller.php',
    'line' => 188,
    'function' => 'install_bases',
    'args' =>
    array (
      0 => 'localhost',
      1 => '',
      2 => '',
      3 => 'sqlite3',
      4 => 'spip',
      5 => 'spip',
      6 => 511,
    ),
  ),

Le `511` à la fin correspond au _SPIP_CHMOD par défaut, que j'utilise lorsque j'appelle `install_bases()`.