Merge pull request #12606 from nupplaphil/bug/file_put

Fix config read/write locking

bin/daemon.php

@@ -126,7 +126,7 @@ if ($mode == 'status') {
 
 	unlink($pidfile);
 
-	DI::config()->set('system', 'worker_daemon_mode', false);
+	DI::keyValue()->set('worker_daemon_mode', false);
 	die("Daemon process $pid isn't running.\n");
 }
 

src/App/BaseURL.php

@@ -172,53 +172,32 @@ class BaseURL
 	 */
 	public function save($hostname = null, $sslPolicy = null, $urlPath = null): bool
 	{
-		$currHostname  = $this->hostname;
-		$currSSLPolicy = $this->sslPolicy;
-		$currURLPath   = $this->urlPath;
+		$currUrl = $this->url;
+
+		$configTransaction = $this->config->beginTransaction();
 
 		if (!empty($hostname) && $hostname !== $this->hostname) {
-			if ($this->config->set('config', 'hostname', $hostname)) {
-				$this->hostname = $hostname;
-			} else {
-				return false;
-			}
+			$configTransaction->set('config', 'hostname', $hostname);
+			$this->hostname = $hostname;
 		}
 
 		if (isset($sslPolicy) && $sslPolicy !== $this->sslPolicy) {
-			if ($this->config->set('system', 'ssl_policy', $sslPolicy)) {
-				$this->sslPolicy = $sslPolicy;
-			} else {
-				$this->hostname = $currHostname;
-				$this->config->set('config', 'hostname', $this->hostname);
-				return false;
-			}
+			$configTransaction->set('system', 'ssl_policy', $sslPolicy);
+			$this->sslPolicy = $sslPolicy;
 		}
 
 		if (isset($urlPath) && $urlPath !== $this->urlPath) {
-			if ($this->config->set('system', 'urlpath', $urlPath)) {
-				$this->urlPath = $urlPath;
-			} else {
-				$this->hostname  = $currHostname;
-				$this->sslPolicy = $currSSLPolicy;
-				$this->config->set('config', 'hostname', $this->hostname);
-				$this->config->set('system', 'ssl_policy', $this->sslPolicy);
-				return false;
-			}
+			$configTransaction->set('system', 'urlpath', $urlPath);
+			$this->urlPath = $urlPath;
 		}
 
 		$this->determineBaseUrl();
-		if (!$this->config->set('system', 'url', $this->url)) {
-			$this->hostname  = $currHostname;
-			$this->sslPolicy = $currSSLPolicy;
-			$this->urlPath   = $currURLPath;
-			$this->determineBaseUrl();
-
-			$this->config->set('config', 'hostname', $this->hostname);
-			$this->config->set('system', 'ssl_policy', $this->sslPolicy);
-			$this->config->set('system', 'urlpath', $this->urlPath);
-			return false;
+		if ($this->url !== $currUrl) {
+			$configTransaction->set('system', 'url', $this->url);
 		}
 
+		$configTransaction->commit();
+
 		return true;
 	}
 

src/Core/Config/Capability/ISetConfigValuesTransactionally.php

@@ -41,8 +41,6 @@ interface ISetConfigValuesTransactionally
 	 * @param mixed  $value  The value to store
 	 *
 	 * @return static the current instance
-	 *
-	 * @throws ConfigPersistenceException In case the persistence layer throws errors
 	 */
 	public function set(string $cat, string $key, $value): self;
 
@@ -54,8 +52,6 @@ interface ISetConfigValuesTransactionally
 	 *
 	 * @return static the current instance
 	 *
-	 * @throws ConfigPersistenceException In case the persistence layer throws errors
-	 *
 	 */
 	public function delete(string $cat, string $key): self;
 

src/Core/Config/Model/ConfigTransaction.php

@@ -37,6 +37,8 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 	protected $cache;
 	/** @var Cache */
 	protected $delCache;
+	/** @var bool field to check if something is to save */
+	protected $changedConfig = false;
 
 	public function __construct(IManageConfigValues $config)
 	{
@@ -70,6 +72,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 	public function set(string $cat, string $key, $value): ISetConfigValuesTransactionally
 	{
 		$this->cache->set($cat, $key, $value, Cache::SOURCE_DATA);
+		$this->changedConfig = true;
 
 		return $this;
 	}
@@ -80,6 +83,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 	{
 		$this->cache->delete($cat, $key);
 		$this->delCache->set($cat, $key, 'deleted');
+		$this->changedConfig = true;
 
 		return $this;
 	}
@@ -87,6 +91,11 @@ class ConfigTransaction implements ISetConfigValuesTransactionally
 	/** {@inheritDoc} */
 	public function commit(): void
 	{
+		// If nothing changed, just do nothing :)
+		if (!$this->changedConfig) {
+			return;
+		}
+
 		try {
 			$newCache = $this->config->getCache()->merge($this->cache);
 			$newCache = $newCache->diff($this->delCache);

src/Core/Config/Util/ConfigFileManager.php

@@ -173,7 +173,21 @@ class ConfigFileManager
 		$filename = $this->configDir . '/' . self::CONFIG_DATA_FILE;
 
 		if (file_exists($filename)) {
-			$dataArray = include $filename;
+
+			$content = '<?php return [];';
+
+			$configStream = fopen($filename, 'r');
+			if (flock($configStream, LOCK_SH)) {
+				$content = fread($configStream, filesize($filename));
+				if (!$content) {
+					throw new ConfigFileException(sprintf('Couldn\'t read file %s', $filename));
+				}
+				flock($configStream, LOCK_UN);
+			}
+
+			fclose($configStream);
+
+			$dataArray = eval('?>' . $content);
 
 			if (!is_array($dataArray)) {
 				throw new ConfigFileException(sprintf('Error loading config file %s', $filename));
@@ -200,8 +214,12 @@ class ConfigFileManager
 			throw new ConfigFileException('config source cannot get encoded');
 		}
 
-		if (!file_put_contents($this->configDir . '/' . self::CONFIG_DATA_FILE, $encodedData)) {
-			throw new ConfigFileException(sprintf('Cannot save data to file %s/%s', $this->configDir, self::CONFIG_DATA_FILE));
+		$configStream = fopen($this->configDir . '/' . self::CONFIG_DATA_FILE, 'w');
+
+		if (flock($configStream, LOCK_EX)) {
+			fwrite($configStream, $encodedData);
+			fflush($configStream);
+			flock($configStream, LOCK_UN);
 		}
 	}
 

tests/src/Core/Config/Cache/ConfigFileManagerTest.php

@@ -407,7 +407,7 @@ class ConfigFileManagerTest extends MockedTest
 
 		$configFileLoader->setupCache($configCache);
 
-		$specialChars = '!"§$%&/()(/&%$\'><?$a,;:[]}{}\\';
+		$specialChars = '!"§$%&/()(/&%$\'><?$a,;:[]}{}\\?¿¿ß';
 
 		// overwrite some data and save it back to the config file
 		$configCache->set('system', 'test', 'it', Cache::SOURCE_DATA);

tests/src/Core/Config/ConfigTransactionTest.php

@@ -9,6 +9,7 @@ use Friendica\Core\Config\Util\ConfigFileManager;
 use Friendica\Core\Config\ValueObject\Cache;
 use Friendica\Test\MockedTest;
 use Friendica\Test\Util\VFSTrait;
+use Mockery\Exception\InvalidCountException;
 
 class ConfigTransactionTest extends MockedTest
 {
@@ -108,4 +109,27 @@ class ConfigTransactionTest extends MockedTest
 		// the whole category should be gone
 		self::assertNull($tempData['delete'] ?? null);
 	}
+
+	/**
+	 * This test asserts that in empty transactions, no saveData is called, thus no config file writing was performed
+	 */
+	public function testNothingToDo()
+	{
+		$this->configFileManager = \Mockery::spy(ConfigFileManager::class);
+
+		$config = new Config($this->configFileManager, new Cache());
+		$configTransaction = new ConfigTransaction($config);
+
+		// commit empty transaction
+		$configTransaction->commit();
+
+		try {
+			$this->configFileManager->shouldNotHaveReceived('saveData');
+		} catch (InvalidCountException $exception) {
+			self::fail($exception);
+		}
+
+		// If not failed, the test ends successfully :)
+		self::assertTrue(true);
+	}
 }

tests/src/Util/BaseURLTest.php

@@ -23,10 +23,25 @@ namespace Friendica\Test\src\Util;
 
 use Friendica\App\BaseURL;
 use Friendica\Core\Config\Capability\IManageConfigValues;
+use Friendica\Core\Config\Capability\ISetConfigValuesTransactionally;
+use Friendica\Core\Config\Model\Config;
+use Friendica\Core\Config\Model\ConfigTransaction;
+use Friendica\Core\Config\Util\ConfigFileManager;
+use Friendica\Core\Config\ValueObject\Cache;
 use Friendica\Test\MockedTest;
+use Friendica\Test\Util\VFSTrait;
 
 class BaseURLTest extends MockedTest
 {
+	use VFSTrait;
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+
+		$this->setUpVfsDir();
+	}
+
 	public function dataDefault()
 	{
 		return [
@@ -231,6 +246,21 @@ class BaseURLTest extends MockedTest
 	public function dataSave()
 	{
 		return [
+			'no_change' => [
+				'input' => [
+					'hostname'  => 'friendica.local',
+					'urlPath'   => 'path',
+					'sslPolicy' => BaseURL::SSL_POLICY_FULL,
+					'url'       => 'https://friendica.local/path',
+					'force_ssl' => true,
+				],
+				'save' => [
+					'hostname'  => 'friendica.local',
+					'urlPath'   => 'path',
+					'sslPolicy' => BaseURL::SSL_POLICY_FULL,
+				],
+				'url' => 'https://friendica.local/path',
+			],
 			'default' => [
 				'input' => [
 					'hostname'  => 'friendica.old',
@@ -315,28 +345,20 @@ class BaseURLTest extends MockedTest
 	 */
 	public function testSave($input, $save, $url)
 	{
-		$configMock = \Mockery::mock(IManageConfigValues::class);
-		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']);
-		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']);
-		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']);
-		$configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']);
-		$configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']);
+		$configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/');
+		$config = new Config($configFileManager, new Cache([
+			'config' => [
+				'hostname' => $input['hostname'] ?? null,
+			],
+			'system' => [
+				'urlpath' => $input['urlPath'] ?? null,
+				'ssl_policy' => $input['sslPolicy'] ?? null,
+				'url' => $input['url'] ?? null,
+				'force_ssl' => $input['force_ssl'] ?? null,
+			],
+		]));
 
-		$baseUrl = new BaseURL($configMock, []);
-
-		if (isset($save['hostname'])) {
-			$configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once();
-		}
-
-		if (isset($save['urlPath'])) {
-			$configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once();
-		}
-
-		if (isset($save['sslPolicy'])) {
-			$configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once();
-		}
-
-		$configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once();
+		$baseUrl = new BaseURL($config, []);
 
 		$baseUrl->save($save['hostname'], $save['sslPolicy'], $save['urlPath']);
 
@@ -353,28 +375,20 @@ class BaseURLTest extends MockedTest
 	 */
 	public function testSaveByUrl($input, $save, $url)
 	{
-		$configMock = \Mockery::mock(IManageConfigValues::class);
-		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn($input['hostname']);
-		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn($input['urlPath']);
-		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn($input['sslPolicy']);
-		$configMock->shouldReceive('get')->with('system', 'url')->andReturn($input['url']);
-		$configMock->shouldReceive('get')->with('system', 'force_ssl')->andReturn($input['force_ssl']);
+		$configFileManager = new ConfigFileManager($this->root->url(), $this->root->url() . '/config/', $this->root->url() . '/static/');
+		$config = new Config($configFileManager, new Cache([
+			'config' => [
+				'hostname' => $input['hostname'] ?? null,
+			],
+			'system' => [
+				'urlpath' => $input['urlPath'] ?? null,
+				'ssl_policy' => $input['sslPolicy'] ?? null,
+				'url' => $input['url'] ?? null,
+				'force_ssl' => $input['force_ssl'] ?? null,
+			],
+		]));
 
-		$baseUrl = new BaseURL($configMock, []);
-
-		if (isset($save['hostname'])) {
-			$configMock->shouldReceive('set')->with('config', 'hostname', $save['hostname'])->andReturn(true)->once();
-		}
-
-		if (isset($save['urlPath'])) {
-			$configMock->shouldReceive('set')->with('system', 'urlpath', $save['urlPath'])->andReturn(true)->once();
-		}
-
-		if (isset($save['sslPolicy'])) {
-			$configMock->shouldReceive('set')->with('system', 'ssl_policy', $save['sslPolicy'])->andReturn(true)->once();
-		}
-
-		$configMock->shouldReceive('set')->with('system', 'url', $url)->andReturn(true)->once();
+		$baseUrl = new BaseURL($config, []);
 
 		$baseUrl->saveByURL($url);
 
@@ -498,65 +512,4 @@ class BaseURLTest extends MockedTest
 
 		self::assertEquals($redirect, $baseUrl->checkRedirectHttps());
 	}
-
-	public function dataWrongSave()
-	{
-		return [
-			'wrongHostname' => [
-				'fail' => 'hostname',
-			],
-			'wrongSSLPolicy' => [
-				'fail' => 'sslPolicy',
-			],
-			'wrongURLPath' => [
-				'fail' => 'urlPath',
-			],
-			'wrongURL' => [
-				'fail' => 'url',
-			],
-		];
-	}
-
-	/**
-	 * Test the save() method with wrong parameters
-	 * @dataProvider dataWrongSave
-	 */
-	public function testWrongSave($fail)
-	{
-		$configMock = \Mockery::mock(IManageConfigValues::class);
-		$configMock->shouldReceive('get')->with('config', 'hostname')->andReturn('friendica.local');
-		$configMock->shouldReceive('get')->with('system', 'urlpath')->andReturn('new/test');
-		$configMock->shouldReceive('get')->with('system', 'ssl_policy')->andReturn(BaseURL::DEFAULT_SSL_SCHEME);
-		$configMock->shouldReceive('get')->with('system', 'url')->andReturn('http://friendica.local/new/test');
-
-		switch ($fail) {
-			case 'hostname':
-				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(false)->once();
-				break;
-			case 'sslPolicy':
-				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(false)->once();
-				break;
-			case 'urlPath':
-				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(false)->once();
-				break;
-			case 'url':
-				$configMock->shouldReceive('set')->with('config', 'hostname', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'ssl_policy', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'urlpath', \Mockery::any())->andReturn(true)->twice();
-				$configMock->shouldReceive('set')->with('system', 'url', \Mockery::any())->andReturn(false)->once();
-				break;
-		}
-
-		$baseUrl = new BaseURL($configMock, []);
-		self::assertFalse($baseUrl->save('test', 10, 'nope'));
-
-		// nothing should have changed because we never successfully saved anything
-		self::assertEquals('friendica.local', $baseUrl->getHostname());
-		self::assertEquals('new/test', $baseUrl->getUrlPath());
-		self::assertEquals(BaseURL::DEFAULT_SSL_SCHEME, $baseUrl->getSSLPolicy());
-		self::assertEquals('http://friendica.local/new/test', $baseUrl->get());
-	}
 }