Erreur json avec la dernière version de Bigup/SPIP 3.2 #4851

Closed
opened 5 months ago by RealET · 14 comments
RealET commented 5 months ago

Bonjour,

En tant d'uploader un PDf de 2125ko, j'ai systématiquement cette erreur avec https://git.spip.net/spip/bigup/src/branch/1.0 à jour sous SPIP 3.2 (à jour git aussi):

Erreur de transfert. : SyntaxError: Unexpected end of JSON input

Bonjour, En tant d'uploader un PDf de 2125ko, j'ai systématiquement cette erreur avec https://git.spip.net/spip/bigup/src/branch/1.0 à jour sous SPIP 3.2 (à jour git aussi): > Erreur de transfert. : SyntaxError: Unexpected end of JSON input
Owner

De ce que je vois, sur un fichier de 4.7Mo, le JS envoie 4 paquets : 1M, 1M, 1M, 1.7M… au lieu de 5 tel que 1M, 1M, 1M, 1M, 1M, .7M

Du coup le php a un peu de mal à suivre.

Y a peut être un report de bug sur la lib JS qui n’a pas suivi ?

Ceci expliquerait pourquoi de la différence du signe < vs <= que j’ai modifié récemment et qui plantait…

De ce que je vois, sur un fichier de 4.7Mo, le JS envoie 4 paquets : 1M, 1M, 1M, 1.7M… au lieu de 5 tel que 1M, 1M, 1M, 1M, 1M, .7M Du coup le php a un peu de mal à suivre. Y a peut être un report de bug sur la lib JS qui n’a pas suivi ? Ceci expliquerait pourquoi de la différence du signe `<` vs `<=` que j’ai modifié récemment et qui plantait…
Poster

Je viens de tester sur la 4.0 avec un up git de bigup : même message d'erreur.

Par contre, sur une 4.0 installée avec SPIP Loader, l'upload se fait bien.

Je viens de tester sur la 4.0 avec un up git de bigup : même message d'erreur. Par contre, sur une 4.0 installée avec SPIP Loader, l'upload se fait bien.
Poster

Après avoir fait pas mal de tests en essayant de revenir au code précédent de BigUp, j'ai découvert qu'il doit y avoir un autre cache au niveau du navigateur qui fait que si ça a planté avec un fichier, même avec la version précédente de BigUp, ça continue à planter.

⇒ Donc, pour tester, il faut en plus changer le nom du fichier sur le disque dur avant upload.

Après avoir fait pas mal de tests en essayant de revenir au code précédent de BigUp, j'ai découvert qu'il doit y avoir un autre cache au niveau du navigateur qui fait que si ça a planté avec un fichier, même avec la version précédente de BigUp, ça continue à planter. ⇒ Donc, pour tester, il faut en plus **changer le nom du fichier** sur le disque dur avant upload.
Poster

Et donc, en :

  • remettant la ligne 293 à for ($i = 1; $i < $numOfChunks; $i++) {
  • renommant mon fichier PDF pour tester de nouveau

Ça marche™ !

Et donc, en : * remettant la ligne 293 à `for ($i = 1; $i < $numOfChunks; $i++) {` * renommant mon fichier PDF pour tester de nouveau Ça marche™ !
Owner

Certes… mais on a justement changé cette ligne pour corriger un autre bug…
Du coup je suis tout perdu là…

Certes… mais on a justement changé cette ligne pour corriger un autre bug… Du coup je suis tout perdu là…
Owner

bon il semble qu’un des soucis soit que les calculs du nombre ne soient pas identiques entre le JS et le PHP…

var chunks = Math.max(
  round(this.size / this.chunkSize), 1
);
$numOfChunks = 
	intval($totalSize / $chunkSize) 
    + ($totalSize % $chunkSize == 0 ? 0 : 1)
;

il semblerait qu’il faille changer le php là dessus… tss.

bon il semble qu’un des soucis soit que les calculs du nombre ne soient pas identiques entre le JS et le PHP… ```js var chunks = Math.max( round(this.size / this.chunkSize), 1 ); ``` ```php $numOfChunks = intval($totalSize / $chunkSize) + ($totalSize % $chunkSize == 0 ? 0 : 1) ; ``` il semblerait qu’il faille changer le php là dessus… tss.
Owner

Bon, donc j’ai a peu près toutes les explications je pense.

Le commit 509ef131ff était insuffisant.

Côté JS

De ce que j’ai compris, flow.js découpe le fichier en N chunks (morceaux) de taille T (disons 1Mo).

Pour le dernier morceau il a 2 choix de stratégie avec une option forceChunkSize (par défaut à false) :

  • false : le dernier chunk fait au minimum T, et au maximum 2 * T
  • true : le dernier chunk fait entre 0 et T.

Autrement dit, le dernier chunk est soit >= T (false), soit <= T (true).
Cela change le nombre total N de chunks.

Pour un fichier de 4.7Mo on aurait donc

  • false : 4 chunks : 1 - 1 - 1 - 1.7
  • true : 5 chunks : 1 - 1 - 1 - 1 - .7

Si le fichier fait 4Mo tout rond, les deux options découpent en 4 chunks.

On peut supposer déjà que Bigup utilise toujours la même option (mais après tout ce n’est pas forcément obligatoire).

La bonne nouvelle c’est que Flow transmets le nombre de chunks qu’il utilise lors de ses requêtes.

Côté PHP

Côté PHP j’utilisais une formule qui semble t’il ne calculait pas correctement le nombre de chunks.

En l’occurrence il y avait avant 509ef131ff :

$numOfChunks = intval($totalSize / $chunkSize) + ($totalSize % $chunkSize == 0 ? 0 : 1);
for ($i = 1; $i < $numOfChunks; $i++) {
	if (!$this->isChunkUploaded($identifier, $filename, $i)) {
			return false;
	}
}

Je n’ai aucun souvenir du pourquoi ce calcul là présentement (peut être repris d’un code d’exemple quelque part à un moment ?). Toujours est-il que

  • la formule est différente de Flow par défaut, qui ferait : max(floor($totalSize / $chunkSize), 1)
  • on ne testait que n-1 chunks dans ce code (sous entendu qu’on calculait «comme si» le dernier chunk était toujours cumulé à l’avant dernier, ce qui est souvent juste… mais pas tout le temps.

Récacapitulons

TotalSize ChunkSize N Js (false) N PHP
5000 1000 5 5
5001 1000 5 6
4999 1000 4 5
500 1000 1 1
0 1000 1 0

Il y a une différence entre PHP et JS particulièrement sur les fichiers dont la taille est une division entière des chunks. Je pense que c’est ce problème qui a été rencontré par @g0oz alors qu’il créait des fichiers avec truncate -s 5M 5M.png

Bref, ceci permet d’y voir un peu plus clair.

Corrections possibles.

Plein de choix !

  1. corriger le calcul théorique du nombre de chunks par PHP.
$numOfChunks = max(floor($totalSize / $chunkSize), 1);
for ($i = 1; $i <= $numOfChunks; $i++) { }
  1. utiliser… le nombre de chunks envoyé par Flow

Ça a l’avantage de fonctionner quelque soit la stratégie de flow.js du dernier chunk (contrairement au calcul 1) précédent).

$numOfChunks = $this->_request('totalChunks');
for ($i = 1; $i <= $numOfChunks; $i++) { }
  1. calculer soi-même si la taille du fichier est suffisante

Mais ça ajoute des accès disques plus nombreux (filesize)

	/**
	 * Indique si tous les morceaux d'un fichier ont été reçus
	 *
	 * @param string $filename
	 * @param string $identifier
	 * @param int $chunkSize
	 * @param int $totalSize
	 * @return bool
	**/
	public function isFileUploadComplete($filename, $identifier, $chunkSize, $totalSize) {
		$chunksDir = $this->cache->parts->fichiers->dir_fichier($identifier, $filename);
		$chunks = $this->getChunkFiles($chunksDir);
		$size = 0;
		foreach ($chunks as $chunk) {
			$size += filesize($chunk);
		}
		if ($size >= $totalSize) {
			return true;
		}
		return false;
	}
Bon, donc j’ai a peu près toutes les explications je pense. Le commit 509ef131ffff6 était insuffisant. ### Côté JS De ce que j’ai compris, flow.js découpe le fichier en N chunks (morceaux) de taille T (disons 1Mo). Pour le dernier morceau il a 2 choix de stratégie avec une option `forceChunkSize` (par défaut à false) : - false : le dernier chunk fait au minimum T, et au maximum 2 * T - true : le dernier chunk fait entre 0 et T. Autrement dit, le dernier chunk est soit >= T (false), soit <= T (true). Cela change le nombre total N de chunks. Pour un fichier de 4.7Mo on aurait donc - false : 4 chunks : 1 - 1 - 1 - 1.7 - true : 5 chunks : 1 - 1 - 1 - 1 - .7 Si le fichier fait 4Mo tout rond, les deux options découpent en 4 chunks. On peut supposer déjà que Bigup utilise toujours la même option (mais après tout ce n’est pas forcément obligatoire). La bonne nouvelle c’est que Flow transmets le nombre de chunks qu’il utilise lors de ses requêtes. ### Côté PHP Côté PHP j’utilisais une formule qui semble t’il ne calculait pas correctement le nombre de chunks. En l’occurrence il y avait avant 509ef131ffff6 : ```php $numOfChunks = intval($totalSize / $chunkSize) + ($totalSize % $chunkSize == 0 ? 0 : 1); for ($i = 1; $i < $numOfChunks; $i++) { if (!$this->isChunkUploaded($identifier, $filename, $i)) { return false; } } ``` Je n’ai aucun souvenir du pourquoi ce calcul là présentement (peut être repris d’un code d’exemple quelque part à un moment ?). Toujours est-il que - la formule est différente de Flow par défaut, qui ferait : `max(floor($totalSize / $chunkSize), 1)` - on ne testait que n-1 chunks dans ce code (sous entendu qu’on calculait «comme si» le dernier chunk était toujours cumulé à l’avant dernier, ce qui est souvent juste… mais pas tout le temps. Récacapitulons | TotalSize | ChunkSize | N Js (false) | N PHP | | --------- | --------- | ------------ | ----- | | 5000 | 1000 | 5 | 5 | | 5001 | 1000 | 5 | 6 | | 4999 | 1000 | 4 | 5 | | 500 | 1000 | 1 | 1 | | 0 | 1000 | 1 | 0 | Il y a une différence entre PHP et JS particulièrement sur les fichiers dont la taille est une division entière des chunks. Je pense que c’est ce problème qui a été rencontré par @g0oz alors qu’il créait des fichiers avec `truncate -s 5M 5M.png` Bref, ceci permet d’y voir un peu plus clair. ### Corrections possibles. Plein de choix ! 1) corriger le calcul théorique du nombre de chunks par PHP. ```php $numOfChunks = max(floor($totalSize / $chunkSize), 1); for ($i = 1; $i <= $numOfChunks; $i++) { } ``` 2) utiliser… le nombre de chunks envoyé par Flow Ça a l’avantage de fonctionner quelque soit la stratégie de flow.js du dernier chunk (contrairement au calcul 1) précédent). ```php $numOfChunks = $this->_request('totalChunks'); for ($i = 1; $i <= $numOfChunks; $i++) { } ``` 3) calculer soi-même si la taille du fichier est suffisante Mais ça ajoute des accès disques plus nombreux (filesize) ```php /** * Indique si tous les morceaux d'un fichier ont été reçus * * @param string $filename * @param string $identifier * @param int $chunkSize * @param int $totalSize * @return bool **/ public function isFileUploadComplete($filename, $identifier, $chunkSize, $totalSize) { $chunksDir = $this->cache->parts->fichiers->dir_fichier($identifier, $filename); $chunks = $this->getChunkFiles($chunksDir); $size = 0; foreach ($chunks as $chunk) { $size += filesize($chunk); } if ($size >= $totalSize) { return true; } return false; } ```

Merci pour cette explication argumentée !

Il me semble que la proposition 3 est la plus robuste, on n'est moins sensible à une modification du calcul du nombre de chunck par flow. Si nous branchons une librairie pour redimmensionner les photos coté client, nous serons moins impactés également.

Merci pour cette explication argumentée ! Il me semble que la proposition 3 est la plus robuste, on n'est moins sensible à une modification du calcul du nombre de chunck par flow. Si nous branchons une librairie pour redimmensionner les photos coté client, nous serons moins impactés également.

Sans hésiter la 1) car c'est la seule manière """correcte/logique""" de faire sans dégrader la sécurité et/ou les performances.

Mais quand je lis le tableau que tu donnes ; c'est plutôt JS qui ne compte "pas droit" et qu'il faudrait corriger en fixant simplement "forceChunkSize" a true non ?

Sans hésiter la 1) car c'est la seule manière """correcte/logique""" de faire sans dégrader la sécurité et/ou les performances. Mais quand je lis le tableau que tu donnes ; c'est plutôt JS qui ne compte "pas droit" et qu'il faudrait corriger en fixant simplement "forceChunkSize" a true non ?

La 2) puisque tu n'envisages pas de modifier le js de flow et qu'il va bien falloir faire avec comme il fait lui :-)

La 2) puisque tu n'envisages pas de modifier le js de flow et qu'il va bien falloir faire avec comme il fait lui :-)
Owner

Je propose un mix entre 2 et 3 : On ne vérifie la taille que si on pense avoir obtenu tous les morceaux. Ça limite donc cette vérification de taille à la réception du dernier morceau.

Je propose un mix entre 2 et 3 : On ne vérifie la taille que si on pense avoir obtenu tous les morceaux. Ça limite donc cette vérification de taille à la réception du dernier morceau.

Ca semble résoudre le problème chez moi.

Ca semble résoudre le problème chez moi.
b_b added the
bug
label 5 months ago
b_b added this to the spip-4.0 milestone 5 months ago
Owner

To be verified.
Tu confirmes que c’est bon @RealET ?

To be verified. Tu confirmes que c’est bon @RealET ?
Poster

To be verified.
Tu confirmes que c’est bon @RealET ?

Je confirme que c'est bon avec le fichier qui posait problème.
Tu peux passer le paquet en 1.0.10 et poser un tag ;-)

> To be verified. > Tu confirmes que c’est bon @RealET ? Je confirme que c'est bon avec le fichier qui posait problème. Tu peux passer le paquet en 1.0.10 et poser un tag ;-)
marcimat closed this issue 5 months ago
Sign in to join this conversation.
No Milestone
No Assignees
6 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.