From 5d83f4670ab787e8b448db1a3752b04d1f52bafa Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Thu, 15 Apr 2021 21:57:23 +0100 Subject: [PATCH] RegionLoader: Switch to using named constructors this makes the code more self-descriptive, and also helps to detect potential bugs. --- src/world/format/io/region/RegionLoader.php | 43 +++++++++++++------ .../format/io/region/RegionWorldProvider.php | 4 +- .../format/io/region/RegionLoaderTest.php | 27 ++++++++++-- tools/compact-regions.php | 4 +- 4 files changed, 56 insertions(+), 22 deletions(-) diff --git a/src/world/format/io/region/RegionLoader.php b/src/world/format/io/region/RegionLoader.php index 746e057f3..dd50f6c38 100644 --- a/src/world/format/io/region/RegionLoader.php +++ b/src/world/format/io/region/RegionLoader.php @@ -83,30 +83,45 @@ class RegionLoader{ /** * @throws CorruptedRegionException */ - public function __construct(string $filePath){ + private function __construct(string $filePath){ $this->filePath = $filePath; $this->garbageTable = new RegionGarbageMap([]); - - clearstatcache(false, $this->filePath); - $exists = file_exists($this->filePath); - if(!$exists){ - touch($this->filePath); - }elseif(filesize($this->filePath) % 4096 !== 0){ - throw new CorruptedRegionException("Region file should be padded to a multiple of 4KiB"); - } + $this->lastUsed = time(); $filePointer = fopen($this->filePath, "r+b"); if($filePointer === false) throw new AssumptionFailedError("fopen() should not fail here"); $this->filePointer = $filePointer; stream_set_read_buffer($this->filePointer, 1024 * 16); //16KB stream_set_write_buffer($this->filePointer, 1024 * 16); //16KB - if(!$exists){ - $this->createBlank(); - }else{ - $this->loadLocationTable(); + } + + /** + * @throws CorruptedRegionException + */ + public static function loadExisting(string $filePath) : self{ + clearstatcache(false, $filePath); + if(!file_exists($filePath)){ + throw new \RuntimeException("File $filePath does not exist"); + } + if(filesize($filePath) % 4096 !== 0){ + throw new CorruptedRegionException("Region file should be padded to a multiple of 4KiB"); } - $this->lastUsed = time(); + $result = new self($filePath); + $result->loadLocationTable(); + return $result; + } + + public static function createNew(string $filePath) : self{ + clearstatcache(false, $filePath); + if(file_exists($filePath)){ + throw new \RuntimeException("Region file $filePath already exists"); + } + touch($filePath); + + $result = new self($filePath); + $result->createBlank(); + return $result; } public function __destruct(){ diff --git a/src/world/format/io/region/RegionWorldProvider.php b/src/world/format/io/region/RegionWorldProvider.php index 67d2f362f..212691b28 100644 --- a/src/world/format/io/region/RegionWorldProvider.php +++ b/src/world/format/io/region/RegionWorldProvider.php @@ -114,7 +114,7 @@ abstract class RegionWorldProvider extends BaseWorldProvider{ $path = $this->pathToRegion($regionX, $regionZ); try{ - $this->regions[$index] = new RegionLoader($path); + $this->regions[$index] = RegionLoader::loadExisting($path); }catch(CorruptedRegionException $e){ $logger = \GlobalLogger::get(); $logger->error("Corrupted region file detected: " . $e->getMessage()); @@ -123,7 +123,7 @@ abstract class RegionWorldProvider extends BaseWorldProvider{ rename($path, $backupPath); $logger->error("Corrupted region file has been backed up to " . $backupPath); - $this->regions[$index] = new RegionLoader($path); //this will create a new empty region to replace the corrupted one + $this->regions[$index] = RegionLoader::createNew($path); } } return $this->regions[$index]; diff --git a/tests/phpunit/world/format/io/region/RegionLoaderTest.php b/tests/phpunit/world/format/io/region/RegionLoaderTest.php index 2b35f2f19..0de5babc1 100644 --- a/tests/phpunit/world/format/io/region/RegionLoaderTest.php +++ b/tests/phpunit/world/format/io/region/RegionLoaderTest.php @@ -25,11 +25,14 @@ namespace pocketmine\world\format\io\region; use PHPUnit\Framework\TestCase; use pocketmine\world\format\ChunkException; +use function bin2hex; +use function clearstatcache; use function file_exists; use function random_bytes; use function str_repeat; use function sys_get_temp_dir; use function unlink; +use const DIRECTORY_SEPARATOR; class RegionLoaderTest extends TestCase{ @@ -43,7 +46,7 @@ class RegionLoaderTest extends TestCase{ if(file_exists($this->regionPath)){ unlink($this->regionPath); } - $this->region = new RegionLoader($this->regionPath); + $this->region = RegionLoader::createNew($this->regionPath); } public function tearDown() : void{ @@ -63,7 +66,7 @@ class RegionLoaderTest extends TestCase{ $this->region->writeChunk(0, 0, $data); $this->region->close(); - $r = new RegionLoader($this->regionPath); + $r = RegionLoader::loadExisting($this->regionPath); self::assertSame($data, $r->readChunk(0, 0)); } @@ -119,11 +122,27 @@ class RegionLoaderTest extends TestCase{ */ public function testRegionHeaderCachedFilesizeRegression() : void{ $this->region->close(); - $region = new RegionLoader($this->regionPath); //now we have a region, so the header will be verified, triggering two filesize() calls + $region = RegionLoader::loadExisting($this->regionPath); //now we have a region, so the header will be verified, triggering two filesize() calls $data = str_repeat("hello", 2000); $region->writeChunk(0, 0, $data); //add some data to the end of the file, to make the cached filesize invalid $region->close(); - $region = new RegionLoader($this->regionPath); + $region = RegionLoader::loadExisting($this->regionPath); self::assertSame($data, $region->readChunk(0, 0)); } + + public function testCreateNewWithExistingRegion() : void{ + $this->region->close(); + $this->expectException(\RuntimeException::class); + RegionLoader::createNew($this->regionPath); + } + + public function testLoadExistingWithMissingFile() : void{ + clearstatcache(); + + do{ + $randfile = sys_get_temp_dir() . DIRECTORY_SEPARATOR . bin2hex(random_bytes(6)) . ".mca"; + }while(file_exists($randfile)); + $this->expectException(\RuntimeException::class); + RegionLoader::loadExisting($randfile); + } } diff --git a/tools/compact-regions.php b/tools/compact-regions.php index 77089ce0d..54e6fbe94 100644 --- a/tools/compact-regions.php +++ b/tools/compact-regions.php @@ -108,7 +108,7 @@ function main(array $argv) : int{ $totalCount = count($files); foreach($files as $file => $size){ try{ - $oldRegion = new RegionLoader($file); + $oldRegion = RegionLoader::loadExisting($file); }catch(CorruptedRegionException $e){ $logger->error("Damaged region in file $file (" . $e->getMessage() . "), skipping"); $corruptedFiles[] = $file; @@ -117,7 +117,7 @@ function main(array $argv) : int{ } $newFile = $file . ".compacted"; - $newRegion = new RegionLoader($newFile); + $newRegion = RegionLoader::createNew($newFile); $emptyRegion = true; $corruption = false;