Parcourir la source

Reduce memory consumption of export (#9350)

Contributes to #6139 (export part; the import will follow in another PR)

Exporting favourites was collecting the whole file in memory. With many articles, it fails with the default PHP memory limit.

## Changes
- New `Minz_View::helperToFile()`, similar to `helperToString()`, writing the output of a helper into a file by chunks of 64 KiB instead of keeping it in memory.
- `FreshRSS_Export_Service`: the `generate*()` methods now return the path of a temporary file instead of the content, and `zip()` builds the archive from those files (`ZipArchive::addFile()` instead of `addFromString()`). Temporary files are created in `TMP_PATH` (already required to be writable) and deleted at the end of the script.
- The export is sent with `readfile()`: Web UI, `cli/export-zip-for-user.php`, `cli/export-opml-for-user.php` and the OPML export of the Google Reader API.
- Note: this changes what `FreshRSS_Export_Service` returns (a path instead of the content). All callers in the repository are updated, and I could not find any public extension using this service.

## Measurements
Peak PHP memory (`memory_get_peak_usage(true)`), SQLite database with synthetic starred articles:

| Scenario | Before | After |
|---|---|---|
| Web, export your favourites, 10,000 articles (69 MB JSON) | 136 MB (fails with the default `memory_limit` of 128 MB) | 6 MB |
| Web, ZIP (OPML + favourites + labelled articles + 4 feeds), 10,000 articles | 152 MB | 6 MB |
| CLI `export-zip-for-user.php`, 10,000 articles | 189 MB (fails with 128 MB) | 8 MB |
| Web, export your favourites, 30,000 articles (208 MB JSON) | 404 MB | 10 MB |

Co-authored-by: Alejandro Borrego <210245044+abp002@users.noreply.github.com>
abp002 il y a 5 jours
Parent
commit
48547b4a6c

+ 14 - 14
app/Controllers/importExportController.php

@@ -788,21 +788,21 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 		$exported_files = [];
 		$exported_files = [];
 
 
 		if ($export_opml) {
 		if ($export_opml) {
-			[$filename, $content] = $export_service->generateOpml();
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $export_service->generateOpml();
+			$exported_files[$filename] = $path;
 		}
 		}
 
 
 		// Starred and labelled entries are merged in the same `starred` file
 		// Starred and labelled entries are merged in the same `starred` file
 		// to avoid duplication of content.
 		// to avoid duplication of content.
 		if ($export_starred && $export_labelled) {
 		if ($export_starred && $export_labelled) {
-			[$filename, $content] = $export_service->generateStarredEntries('ST');
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $export_service->generateStarredEntries('ST');
+			$exported_files[$filename] = $path;
 		} elseif ($export_starred) {
 		} elseif ($export_starred) {
-			[$filename, $content] = $export_service->generateStarredEntries('S');
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $export_service->generateStarredEntries('S');
+			$exported_files[$filename] = $path;
 		} elseif ($export_labelled) {
 		} elseif ($export_labelled) {
-			[$filename, $content] = $export_service->generateStarredEntries('T');
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $export_service->generateStarredEntries('T');
+			$exported_files[$filename] = $path;
 		}
 		}
 
 
 		foreach ($export_feeds as $feed_id) {
 		foreach ($export_feeds as $feed_id) {
@@ -812,8 +812,8 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 				continue;
 				continue;
 			}
 			}
 
 
-			[$filename, $content] = $result;
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $result;
+			$exported_files[$filename] = $path;
 		}
 		}
 
 
 		$nb_files = count($exported_files);
 		$nb_files = count($exported_files);
@@ -826,7 +826,7 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 		if ($nb_files === 1) {
 		if ($nb_files === 1) {
 			// If we only have one file, we just export it as it is
 			// If we only have one file, we just export it as it is
 			$filename = key($exported_files);
 			$filename = key($exported_files);
-			$content = $exported_files[$filename];
+			$path = $exported_files[$filename];
 		} else {
 		} else {
 			// More files? Let’s compress them in a Zip archive
 			// More files? Let’s compress them in a Zip archive
 			if (!extension_loaded('zip')) {
 			if (!extension_loaded('zip')) {
@@ -838,10 +838,10 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 				return;
 				return;
 			}
 			}
 
 
-			[$filename, $content] = $export_service->zip($exported_files);
+			[$filename, $path] = $export_service->zip($exported_files);
 		}
 		}
 
 
-		if (!is_string($content)) {
+		if (!is_string($path)) {
 			Minz_Request::bad(_t('feedback.import_export.zip_error'), ['c' => 'importExport', 'a' => 'index']);
 			Minz_Request::bad(_t('feedback.import_export.zip_error'), ['c' => 'importExport', 'a' => 'index']);
 			return;
 			return;
 		}
 		}
@@ -851,7 +851,7 @@ class FreshRSS_importExport_Controller extends FreshRSS_ActionController {
 		header('Content-disposition: attachment; filename="' . $filename . '"');
 		header('Content-disposition: attachment; filename="' . $filename . '"');
 
 
 		$this->view->_layout(null);
 		$this->view->_layout(null);
-		$this->view->content = $content;
+		$this->view->exportPath = $path;
 	}
 	}
 
 
 	/**
 	/**

+ 1 - 0
app/Models/View.php

@@ -91,6 +91,7 @@ class FreshRSS_View extends Minz_View {
 
 
 	// Export / Import
 	// Export / Import
 	public string $content;
 	public string $content;
+	public string $exportPath;
 	public int $feedCount;
 	public int $feedCount;
 	/** @var array<string,array<string>> */
 	/** @var array<string,array<string>> */
 	public array $entryIdsTagNames = [];
 	public array $entryIdsTagNames = [];

+ 48 - 29
app/Services/ExportService.php

@@ -40,8 +40,9 @@ class FreshRSS_Export_Service {
 	}
 	}
 
 
 	/**
 	/**
-	 * Generate OPML file content.
-	 * @return array{0:string,1:string} First item is the filename, second item is the content
+	 * Generate OPML file.
+	 * @return array{0:string,1:string} First item is the filename, second item is the path of a temporary file with the content
+	 * @throws Minz_PermissionDeniedException
 	 */
 	 */
 	public function generateOpml(): array {
 	public function generateOpml(): array {
 		$view = new FreshRSS_View();
 		$view = new FreshRSS_View();
@@ -52,12 +53,12 @@ class FreshRSS_Export_Service {
 
 
 		return [
 		return [
 			"feeds_{$day}.opml.xml",
 			"feeds_{$day}.opml.xml",
-			$view->helperToString('export/opml')
+			self::helperToTempFile($view, 'export/opml'),
 		];
 		];
 	}
 	}
 
 
 	/**
 	/**
-	 * Generate the starred and labelled entries file content.
+	 * Generate the starred and labelled entries file.
 	 *
 	 *
 	 * Both starred and labelled entries are put into a "starred" file, that’s
 	 * Both starred and labelled entries are put into a "starred" file, that’s
 	 * why there is only one method for both.
 	 * why there is only one method for both.
@@ -67,7 +68,8 @@ class FreshRSS_Export_Service {
 	 *     'S' (starred/favourite),
 	 *     'S' (starred/favourite),
 	 *     'T' (taggued/labelled),
 	 *     'T' (taggued/labelled),
 	 *     'ST' (starred or labelled)
 	 *     'ST' (starred or labelled)
-	 * @return array{0:string,1:string} First item is the filename, second item is the content
+	 * @return array{0:string,1:string} First item is the filename, second item is the path of a temporary file with the content
+	 * @throws Minz_PermissionDeniedException
 	 */
 	 */
 	public function generateStarredEntries(string $type): array {
 	public function generateStarredEntries(string $type): array {
 		$view = new FreshRSS_View();
 		$view = new FreshRSS_View();
@@ -85,14 +87,15 @@ class FreshRSS_Export_Service {
 
 
 		return [
 		return [
 			"starred_{$day}.json",
 			"starred_{$day}.json",
-			$view->helperToString('export/articles')
+			self::helperToTempFile($view, 'export/articles'),
 		];
 		];
 	}
 	}
 
 
 	/**
 	/**
-	 * Generate the entries file content for the given feed.
-	 * @return array{0:string,1:string}|null First item is the filename, second item is the content.
+	 * Generate the entries file for the given feed.
+	 * @return array{0:string,1:string}|null First item is the filename, second item is the path of a temporary file with the content.
 	 *                    It also can return null if the feed doesn’t exist.
 	 *                    It also can return null if the feed doesn’t exist.
+	 * @throws Minz_PermissionDeniedException
 	 */
 	 */
 	public function generateFeedEntries(int $feed_id, int $max_number_entries): ?array {
 	public function generateFeedEntries(int $feed_id, int $max_number_entries): ?array {
 		$view = new FreshRSS_View();
 		$view = new FreshRSS_View();
@@ -120,13 +123,14 @@ class FreshRSS_Export_Service {
 
 
 		return [
 		return [
 			$filename,
 			$filename,
-			$view->helperToString('export/articles')
+			self::helperToTempFile($view, 'export/articles'),
 		];
 		];
 	}
 	}
 
 
 	/**
 	/**
-	 * Generate the entries file content for all the feeds.
-	 * @return array<string,string> Keys are filenames and values are contents.
+	 * Generate the entries files for all the feeds.
+	 * @return array<string,string> Keys are filenames and values are paths of temporary files with the contents.
+	 * @throws Minz_PermissionDeniedException
 	 */
 	 */
 	public function generateAllFeedEntries(int $max_number_entries): array {
 	public function generateAllFeedEntries(int $max_number_entries): array {
 		$feed_ids = $this->feed_dao->listFeedsIds();
 		$feed_ids = $this->feed_dao->listFeedsIds();
@@ -138,8 +142,8 @@ class FreshRSS_Export_Service {
 				continue;
 				continue;
 			}
 			}
 
 
-			[$filename, $content] = $result;
-			$exported_files[$filename] = $content;
+			[$filename, $path] = $result;
+			$exported_files[$filename] = $path;
 		}
 		}
 
 
 		return $exported_files;
 		return $exported_files;
@@ -147,34 +151,49 @@ class FreshRSS_Export_Service {
 
 
 	/**
 	/**
 	 * Compress several files in a Zip file.
 	 * Compress several files in a Zip file.
-	 * @param array<string,string> $files where the key is the filename, the value is the content
-	 * @return array{0:string,1:string|false} First item is the zip filename, second item is the zip content
+	 * @param array<string,string> $files where the key is the filename, the value is the path of the file
+	 * @return array{0:string,1:string|false} First item is the zip filename, second item is the path of a temporary zip file, or false in case of error
+	 * @throws Minz_PermissionDeniedException
 	 */
 	 */
 	public function zip(array $files): array {
 	public function zip(array $files): array {
 		$day = date('Y-m-d');
 		$day = date('Y-m-d');
 		$zip_filename = 'freshrss_' . $this->username . '_' . $day . '_export.zip';
 		$zip_filename = 'freshrss_' . $this->username . '_' . $day . '_export.zip';
 
 
-		// From https://stackoverflow.com/questions/1061710/php-zip-files-on-the-fly
-		$zip_file = tempnam(TMP_PATH, 'zip');
-		if ($zip_file === false) {
-			return [$zip_filename, false];
-		}
+		$zip_file = self::tempFile();
 		$zip_archive = new ZipArchive();
 		$zip_archive = new ZipArchive();
 		$zip_archive->open($zip_file, ZipArchive::OVERWRITE);
 		$zip_archive->open($zip_file, ZipArchive::OVERWRITE);
 
 
-		foreach ($files as $filename => $content) {
-			$zip_archive->addFromString($filename, $content);
+		foreach ($files as $filename => $path) {
+			$zip_archive->addFile($path, $filename);
 		}
 		}
 
 
-		$zip_archive->close();
-
-		$content = file_get_contents($zip_file);
-
-		unlink($zip_file);
-
 		return [
 		return [
 			$zip_filename,
 			$zip_filename,
-			$content,
+			$zip_archive->close() ? $zip_file : false,
 		];
 		];
 	}
 	}
+
+	/**
+	 * Create a temporary file, which is deleted at the end of the script.
+	 * @throws Minz_PermissionDeniedException
+	 */
+	private static function tempFile(): string {
+		$filename = tempnam(TMP_PATH, 'export');
+		if ($filename === false) {
+			throw new Minz_PermissionDeniedException(TMP_PATH);
+		}
+		register_shutdown_function(static fn() => @unlink($filename));
+		return $filename;
+	}
+
+	/**
+	 * Render a view helper into a temporary file, instead of keeping the whole content in memory.
+	 * @return string The path of the temporary file
+	 * @throws Minz_PermissionDeniedException
+	 */
+	private static function helperToTempFile(FreshRSS_View $view, string $helper): string {
+		$filename = self::tempFile();
+		$view->helperToFile($helper, $filename);
+		return $filename;
+	}
 }
 }

+ 2 - 1
app/views/importExport/export.phtml

@@ -1,4 +1,5 @@
 <?php
 <?php
 declare(strict_types=1);
 declare(strict_types=1);
 /** @var FreshRSS_View $this */
 /** @var FreshRSS_View $this */
-echo $this->content;
+@ob_end_clean();	// Ensure no buffer
+readfile($this->exportPath);

+ 2 - 2
cli/export-opml-for-user.php

@@ -23,8 +23,8 @@ $username = cliInitUser($cliOptions->user);
 fwrite(STDERR, 'FreshRSS exporting OPML for user “' . $username . "”…\n");
 fwrite(STDERR, 'FreshRSS exporting OPML for user “' . $username . "”…\n");
 
 
 $export_service = new FreshRSS_Export_Service($username);
 $export_service = new FreshRSS_Export_Service($username);
-[$filename, $content] = $export_service->generateOpml();
-echo $content;
+[$filename, $path] = $export_service->generateOpml();
+readfile($path);
 
 
 invalidateHttpCache($username);
 invalidateHttpCache($username);
 
 

+ 9 - 6
cli/export-zip-for-user.php

@@ -33,12 +33,12 @@ $number_entries = $cliOptions->maxFeedEntries;
 $exported_files = [];
 $exported_files = [];
 
 
 // First, we generate the OPML file
 // First, we generate the OPML file
-[$filename, $content] = $export_service->generateOpml();
-$exported_files[$filename] = $content;
+[$filename, $path] = $export_service->generateOpml();
+$exported_files[$filename] = $path;
 
 
 // Then, labelled and starred entries
 // Then, labelled and starred entries
-[$filename, $content] = $export_service->generateStarredEntries('ST');
-$exported_files[$filename] = $content;
+[$filename, $path] = $export_service->generateStarredEntries('ST');
+$exported_files[$filename] = $path;
 
 
 // And a list of entries based on the complete list of feeds
 // And a list of entries based on the complete list of feeds
 $feeds_exported_files = $export_service->generateAllFeedEntries($number_entries);
 $feeds_exported_files = $export_service->generateAllFeedEntries($number_entries);
@@ -46,8 +46,11 @@ $exported_files = array_merge($exported_files, $feeds_exported_files);
 
 
 // Finally, we compress all these files into a single Zip archive and we output
 // Finally, we compress all these files into a single Zip archive and we output
 // the content
 // the content
-[$filename, $content] = $export_service->zip($exported_files);
-echo $content;
+[$filename, $path] = $export_service->zip($exported_files);
+if ($path === false) {
+	fail('FreshRSS error: cannot create the Zip archive!');
+}
+readfile($path);
 
 
 invalidateHttpCache($username);
 invalidateHttpCache($username);
 
 

+ 27 - 0
lib/Minz/View.php

@@ -157,6 +157,33 @@ class Minz_View {
 		return ob_get_clean() ?: '';
 		return ob_get_clean() ?: '';
 	}
 	}
 
 
+	/**
+	 * Writes renderHelper() into a file, without keeping the whole output in memory
+	 * @param string $helper the element to be treated
+	 * @param string $filename the path of the file to write
+	 * @throws Minz_PermissionDeniedException
+	 */
+	public function helperToFile(string $helper, string $filename): void {
+		$file = fopen($filename, 'wb');
+		if ($file === false) {
+			throw new Minz_PermissionDeniedException($filename);
+		}
+		$written = true;
+		ob_start(static function (string $buffer) use ($file, &$written): string {
+			$written = $written && fwrite($file, $buffer) !== false;
+			return '';
+		}, 65536);	// Write to the file by chunks of 64 KiB
+		try {
+			$this->renderHelper($helper);
+		} finally {
+			ob_end_flush();
+			$written = fclose($file) && $written;
+		}
+		if (!$written) {
+			throw new Minz_PermissionDeniedException($filename);
+		}
+	}
+
 	/**
 	/**
 	 * Choose the current view layout.
 	 * Choose the current view layout.
 	 * @param string|null $layout the layout name to use, null to use no layouts.
 	 * @param string|null $layout the layout name to use, null to use no layouts.

+ 2 - 2
p/api/greader.php

@@ -315,10 +315,10 @@ final class GReaderAPI {
 	private static function subscriptionExport(): never {
 	private static function subscriptionExport(): never {
 		$user = Minz_User::name() ?? Minz_User::INTERNAL_USER;
 		$user = Minz_User::name() ?? Minz_User::INTERNAL_USER;
 		$export_service = new FreshRSS_Export_Service($user);
 		$export_service = new FreshRSS_Export_Service($user);
-		[$filename, $content] = $export_service->generateOpml();
+		[$filename, $path] = $export_service->generateOpml();
 		header('Content-Type: application/xml; charset=UTF-8');
 		header('Content-Type: application/xml; charset=UTF-8');
 		header('Content-disposition: attachment; filename="' . $filename . '"');
 		header('Content-disposition: attachment; filename="' . $filename . '"');
-		echo $content;
+		readfile($path);
 		exit();
 		exit();
 	}
 	}
 
 

+ 77 - 0
tests/app/Views/exportArticlesTest.php

@@ -0,0 +1,77 @@
+<?php
+declare(strict_types=1);
+
+final class exportArticlesTest extends \PHPUnit\Framework\TestCase {
+	private string $filename;
+
+	#[\Override]
+	public static function setUpBeforeClass(): void {
+		// `FreshRSS_View` needs a system configuration; the shipped defaults are enough to render an export.
+		Minz_Configuration::register('system', FRESHRSS_PATH . '/config.default.php', FRESHRSS_PATH . '/config.default.php');
+	}
+
+	#[\Override]
+	protected function setUp(): void {
+		$filename = tempnam(sys_get_temp_dir(), 'freshrss_test_');
+		self::assertIsString($filename);
+		$this->filename = $filename;
+	}
+
+	#[\Override]
+	protected function tearDown(): void {
+		@unlink($this->filename);
+	}
+
+	/** @return list<FreshRSS_Entry> */
+	private static function entries(int $count): array {
+		$entries = [];
+		for ($i = 1; $i <= $count; $i++) {
+			$entry = new FreshRSS_Entry(1, 'guid-' . $i, 'Title ' . $i, '', str_repeat('<p>Contenu exporté ' . $i . '</p>', 200),
+				'https://example.net/' . $i, 1700000000 + $i);
+			$entry->_id(1700000000000000 + $i);
+			$entries[] = $entry;
+		}
+		return $entries;
+	}
+
+	/** @param iterable<FreshRSS_Entry> $entries */
+	private static function view(iterable $entries): FreshRSS_View {
+		$view = new FreshRSS_View();
+		$view->internal_rendering = true;
+		$view->list_title = 'Starred';
+		$view->type = 'starred';
+		$view->feed = new FreshRSS_Feed('https://example.net/feed.xml', false);
+		$view->entries = $entries;
+		return $view;
+	}
+
+	public function testHelperToFileWritesTheSameContentAsHelperToString(): void {
+		$expected = self::view(self::entries(50))->helperToString('export/articles');
+		self::assertGreaterThan(65536, strlen($expected), 'The export must be larger than one chunk of the output buffer');
+
+		file_put_contents($this->filename, 'Previous content, to be overwritten');
+		$this->expectOutputString('');	// Nothing must leak to the output
+		self::view(self::entries(50))->helperToFile('export/articles', $this->filename);
+
+		self::assertSame($expected, file_get_contents($this->filename));
+		$json = json_decode($expected, true);
+		self::assertIsArray($json);
+		self::assertIsArray($json['items'] ?? null);
+		self::assertCount(50, $json['items']);
+	}
+
+	public function testHelperToFileClosesTheOutputBufferOnError(): void {
+		$entries = (static function (): Generator {
+			yield from self::entries(1);
+			throw new RuntimeException('Database error');
+		})();
+		$level = ob_get_level();
+		try {
+			self::view($entries)->helperToFile('export/articles', $this->filename);
+			self::fail('The exception must be propagated');
+		} catch (RuntimeException $e) {
+			self::assertSame('Database error', $e->getMessage());
+		}
+		self::assertSame($level, ob_get_level());
+	}
+}