From 2d591c3b7cf3950f111d60855a8aa1ffb7d7c609 Mon Sep 17 00:00:00 2001 From: Zach Borboa Date: Sun, 17 Nov 2019 00:19:34 -0800 Subject: [PATCH] Fix lingering temporary download file when using MultiCurl::addDownload() (#602) --- src/Curl/Curl.php | 9 +- src/Curl/MultiCurl.php | 26 +-- tests/PHPCurlClass/PHPMultiCurlClassTest.php | 160 +++++++++++++++++++ 3 files changed, 182 insertions(+), 13 deletions(-) diff --git a/src/Curl/Curl.php b/src/Curl/Curl.php index 837efde..c1f61dd 100644 --- a/src/Curl/Curl.php +++ b/src/Curl/Curl.php @@ -290,6 +290,7 @@ class Curl */ public function download($url, $mixed_filename) { + // Use tmpfile() or php://temp to avoid "Too many open files" error. if (is_callable($mixed_filename)) { $this->downloadCompleteCallback = $mixed_filename; $this->downloadFileName = null; @@ -302,17 +303,17 @@ class Curl // path. The download request will include header "Range: bytes=$filesize-" which is syntactically valid, // but unsatisfiable. $download_filename = $filename . '.pccdownload'; + $this->downloadFileName = $download_filename; - $mode = 'wb'; // Attempt to resume download only when a temporary download file exists and is not empty. if (is_file($download_filename) && $filesize = filesize($download_filename)) { - $mode = 'ab'; $first_byte_position = $filesize; $range = $first_byte_position . '-'; $this->setOpt(CURLOPT_RANGE, $range); + $this->fileHandle = fopen($download_filename, 'ab'); + } else { + $this->fileHandle = fopen($download_filename, 'wb'); } - $this->downloadFileName = $download_filename; - $this->fileHandle = fopen($download_filename, $mode); // Move the downloaded temporary file to the destination save path. $this->downloadCompleteCallback = function ($instance, $fh) use ($download_filename, $filename) { diff --git a/src/Curl/MultiCurl.php b/src/Curl/MultiCurl.php index 7a3a754..175f50b 100644 --- a/src/Curl/MultiCurl.php +++ b/src/Curl/MultiCurl.php @@ -96,22 +96,30 @@ class MultiCurl // path. The download request will include header "Range: bytes=$filesize-" which is syntactically valid, // but unsatisfiable. $download_filename = $filename . '.pccdownload'; + $this->downloadFileName = $download_filename; - $mode = 'wb'; // Attempt to resume download only when a temporary download file exists and is not empty. if (is_file($download_filename) && $filesize = filesize($download_filename)) { - $mode = 'ab'; $first_byte_position = $filesize; $range = $first_byte_position . '-'; $curl->setOpt(CURLOPT_RANGE, $range); - } - $curl->downloadFileName = $download_filename; - $curl->fileHandle = fopen('php://temp', 'wb'); + $curl->fileHandle = fopen($download_filename, 'ab'); - // Move the downloaded temporary file to the destination save path. - $curl->downloadCompleteCallback = function ($instance, $fh) use ($download_filename) { - file_put_contents($download_filename, stream_get_contents($fh)); - }; + // Move the downloaded temporary file to the destination save path. + $curl->downloadCompleteCallback = function ($instance, $fh) use ($download_filename, $filename) { + // Close the open file handle before renaming the file. + if (is_resource($fh)) { + fclose($fh); + } + + rename($download_filename, $filename); + }; + } else { + $curl->fileHandle = fopen('php://temp', 'wb'); + $curl->downloadCompleteCallback = function ($instance, $fh) use ($filename) { + file_put_contents($filename, stream_get_contents($fh)); + }; + } } $curl->setOpt(CURLOPT_FILE, $curl->fileHandle); diff --git a/tests/PHPCurlClass/PHPMultiCurlClassTest.php b/tests/PHPCurlClass/PHPMultiCurlClassTest.php index 58d25e5..1cb06b8 100644 --- a/tests/PHPCurlClass/PHPMultiCurlClassTest.php +++ b/tests/PHPCurlClass/PHPMultiCurlClassTest.php @@ -2561,6 +2561,166 @@ class MultiCurlTest extends \PHPUnit\Framework\TestCase $multi_curl->start(); } + public function testDownload() + { + // Create and upload a file. + $upload_file_path = \Helper\get_png(); + $uploaded_file_path = \Helper\upload_file_to_server($upload_file_path); + + // Download the file. + $downloaded_file_path = tempnam('/tmp', 'php-curl-class.'); + $multi_curl = new MultiCurl(); + $multi_curl->setHeader('X-DEBUG-TEST', 'download_response'); + $multi_curl->addDownload(Test::TEST_URL . '?' . http_build_query(array( + 'file_path' => $uploaded_file_path, + )), $downloaded_file_path); + $multi_curl->complete(function ($instance) use ($upload_file_path) { + \PHPUnit\Framework\Assert::assertFalse($instance->error); + \PHPUnit\Framework\Assert::assertEquals(md5_file($upload_file_path), $instance->responseHeaders['ETag']); + }); + $multi_curl->start(); + $this->assertNotEquals($uploaded_file_path, $downloaded_file_path); + + $this->assertEquals(filesize($upload_file_path), filesize($downloaded_file_path)); + $this->assertEquals(md5_file($upload_file_path), md5_file($downloaded_file_path)); + + // Remove server file. + \Helper\remove_file_from_server($uploaded_file_path); + + unlink($upload_file_path); + unlink($downloaded_file_path); + $this->assertFalse(file_exists($upload_file_path)); + $this->assertFalse(file_exists($downloaded_file_path)); + } + + public function testDownloadRange() + { + // Create and upload a file. + $filename = \Helper\get_png(); + $uploaded_file_path = \Helper\upload_file_to_server($filename); + + $filesize = filesize($filename); + + foreach (array( + false, + 0, + 1, + 2, + 3, + 5, + 10, + 25, + 50, + $filesize - 3, + $filesize - 2, + $filesize - 1, + + // A partial temporary file having the exact same file size as the complete source file should only + // occur under certain circumstances (almost never). When the download successfully completed, the + // temporary file should have been moved to the download destination save path. However, it is possible + // that a larger file download was interrupted after which the source file was updated and now has the + // exact same file size as the partial temporary. When resuming the download, the range is now + // unsatisfiable as the first byte position exceeds the available range. The entire file should be + // downloaded again. + $filesize - 0, + + // A partial temporary file having a larger file size than the complete source file should only occur + // under certain circumstances. This is possible when a download was interrupted after which the source + // file was updated with a smaller file. When resuming the download, the range is now unsatisfiable as + // the first byte position exceeds the the available range. The entire file should be downloaded again. + $filesize + 1, + $filesize + 2, + $filesize + 3, + + ) as $length) { + $source = Test::TEST_URL; + $destination = \Helper\get_tmp_file_path(); + + // Start with no file. + if ($length === false) { + $this->assertFalse(file_exists($destination)); + + // Start with $length bytes of file. + } else { + // Simulate resuming partially downloaded temporary file. + $partial_filename = $destination . '.pccdownload'; + + if ($length === 0) { + $partial_content = ''; + } else { + $file = fopen($filename, 'rb'); + $partial_content = fread($file, $length); + fclose($file); + } + + // Partial content size should be $length bytes large for testing resume download behavior. + if ($length <= $filesize) { + $this->assertEquals($length, strlen($partial_content)); + + // Partial content should not be larger than the original file size. + } else { + $this->assertEquals($filesize, strlen($partial_content)); + } + + file_put_contents($partial_filename, $partial_content); + $this->assertEquals(strlen($partial_content), strlen(file_get_contents($partial_filename))); + } + + // Download (the remaining bytes of) the file. + $multi_curl = new MultiCurl(); + $multi_curl->setHeader('X-DEBUG-TEST', 'download_file_range'); + $multi_curl->addDownload($source . '?' . http_build_query(array( + 'file_path' => $uploaded_file_path, + )), $destination); + + clearstatcache(); + + $instance_error = false; + $multi_curl->complete(function ($instance) use ($filesize, $length, $destination, &$instance_error) { + $expected_bytes_downloaded = $filesize - min($length, $filesize); + $bytes_downloaded = $instance->responseHeaders['content-length']; + if ($length === false || $length === 0) { + $expected_http_status_code = 200; // 200 OK + \PHPUnit\Framework\Assert::assertEquals($expected_bytes_downloaded, $bytes_downloaded); + } elseif ($length >= $filesize) { + $expected_http_status_code = 416; // 416 Requested Range Not Satisfiable + } else { + $expected_http_status_code = 206; // 206 Partial Content + \PHPUnit\Framework\Assert::assertEquals($expected_bytes_downloaded, $bytes_downloaded); + } + \PHPUnit\Framework\Assert::assertEquals($expected_http_status_code, $instance->httpStatusCode); + $instance_error = $instance->error; + }); + $multi_curl->start(); + + if (!$instance_error) { + $this->assertEquals($filesize, filesize($destination)); + unlink($destination); + $this->assertFalse(file_exists($destination)); + } + } + + // Remove server file. + \Helper\remove_file_from_server($uploaded_file_path); + + unlink($filename); + $this->assertFalse(file_exists($filename)); + } + + public function testDownloadErrorDeleteTemporaryFile() + { + $destination = \Helper\get_tmp_file_path(); + + $multi_curl = new MultiCurl(); + $multi_curl->setHeader('X-DEBUG-TEST', '404'); + $multi_curl->addDownload(Test::TEST_URL, $destination); + $multi_curl->complete(function ($instance) use ($destination) { + \PHPUnit\Framework\Assert::assertFalse(file_exists($instance->getDownloadFileName())); + \PHPUnit\Framework\Assert::assertFalse(file_exists($destination)); + }); + $multi_curl->start(); + } + public function testDownloadCallback() { // Upload a file.