Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add lock function #34

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from
Open

Add lock function #34

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0aebb80
Add lock function
danog Jul 15, 2019
c81957b
Improvements
danog Aug 22, 2019
0b7831d
Fixes
danog Aug 22, 2019
6ecb393
Return Lock
danog Aug 22, 2019
a76abf9
Improved description
danog Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,14 @@
"platform": {
"php": "7.0.0"
}
},
"scripts": {
"check": [
"@cs",
"@test"
],
"cs": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix -v --diff --dry-run",
"cs-fix": "PHP_CS_FIXER_IGNORE_ENV=1 php-cs-fixer fix -v --diff",
"test": "@php -dzend.assertions=1 -dassert.exception=1 ./vendor/bin/phpunit --coverage-text"
}
}
44 changes: 44 additions & 0 deletions lib/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

namespace Amp\File;

use Amp\Delayed;
use Amp\Loop;
use Amp\Promise;
use function Amp\call;

const LOOP_STATE_IDENTIFIER = Driver::class;

Expand Down Expand Up @@ -350,3 +352,45 @@ function put(string $path, string $contents): Promise
{
return filesystem()->put($path, $contents);
}

/**
* Asynchronously lock a file
* Resolves with a callbable that MUST eventually be called in order to release the lock.
*
* @param string $file File to lock
* @param integer $operation Locking mode, one of \LOCK_SH or \LOCK_EX (see PHP flock docs)
* @param integer $polling Polling interval for lock in milliseconds
*
* @return \Amp\Promise Resolves with a callbable that MUST eventually be called in order to release the lock.
danog marked this conversation as resolved.
Show resolved Hide resolved
*/
function lock(string $file, int $operation, int $polling = 100): Promise
danog marked this conversation as resolved.
Show resolved Hide resolved
{
return call(static function () use ($file, $operation, $polling) {
if (!yield exists($file)) {
yield \touch($file);
StatCache::clear($file);
}
if ($operation === \LOCK_UN) {
throw new FilesystemException("Can't unlock like this, call the unlock function returned by the first instance of lock that acquired the lock to unlock");
}
$operation |= LOCK_NB;
$res = \fopen($file, 'c');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we just open the file here instead of the additional touch above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well technically yes, but then the touching wouldn't be async.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the fopen isn't async anyway? I guess we'd have to add the function to Handle to have it really async?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, fopen isn't async, but afaik there is no way to obtain a lock using any of the native async libs supported by amphp.
I thought it'd be nice if we could make at least the touch part of the fopen async.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trowski What's your opinion here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm rather uncomfortable introducing anything that can block when it could be rolled into a simple Task executed using parallel.

do {
$result = \flock($res, $operation, $wouldblock);
if (!$result) {
if (!$wouldblock) {
throw new FilesystemException("Failed acquiring lock on file.");
}
yield new Delayed($polling);
}
} while (!$result);
danog marked this conversation as resolved.
Show resolved Hide resolved

return static function () use (&$res) {
if ($res) {
\flock($res, LOCK_UN);
\fclose($res);
$res = null;
}
};
danog marked this conversation as resolved.
Show resolved Hide resolved
});
}
91 changes: 82 additions & 9 deletions test/HandleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
namespace Amp\File\Test;

use Amp\ByteStream\ClosedException;
use Amp\Delayed;
use Amp\File;
use Amp\PHPUnit\TestCase;
use function Amp\Promise\timeout;

abstract class HandleTest extends TestCase
{
Expand All @@ -24,7 +26,7 @@ abstract protected function execute(callable $cb);
public function testWrite()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");
$this->assertSame(0, $handle->tell());
Expand All @@ -44,7 +46,7 @@ public function testWrite()
public function testEmptyWrite()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";

$handle = yield File\open($path, "c+");
$this->assertSame(0, $handle->tell());
Expand All @@ -59,7 +61,7 @@ public function testEmptyWrite()
public function testWriteAfterClose()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");
yield $handle->close();
Expand All @@ -72,7 +74,7 @@ public function testWriteAfterClose()
public function testDoubleClose()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");
yield $handle->close();
Expand All @@ -83,7 +85,7 @@ public function testDoubleClose()
public function testWriteAfterEnd()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
danog marked this conversation as resolved.
Show resolved Hide resolved
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");
$this->assertSame(0, $handle->tell());
Expand All @@ -97,7 +99,7 @@ public function testWriteAfterEnd()
public function testWriteInAppendMode()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "a+");
$this->assertSame(0, $handle->tell());
Expand Down Expand Up @@ -232,7 +234,78 @@ public function testMode()
yield $handle->close();
});
}

/**
* Try locking file.
*
* @param string $file File
* @param int $mode Locking mode
* @param int $polling Polling interval
* @param int $timeout Lock timeout
* @return void
*/
private function tryLock(string $file, int $mode, int $polling, int $timeout)
{
return timeout(File\lock($file, $mode, $polling), $timeout);
}
public function testExclusiveLock()
{
$this->execute(function () {
$primary = null;
$secondary = null;
try {
try {
$primary = yield $this->tryLock(__FILE__, \LOCK_EX, 100, 100);
$this->assertInstanceOf("\\Closure", $primary);
danog marked this conversation as resolved.
Show resolved Hide resolved

$unlocked = false;
$try = $this->tryLock(__FILE__, LOCK_SH, 100, 10000);
$try->onResolve(static function ($e, $secondaryUnlock) use (&$unlocked, &$secondary) {
if ($e) {
throw $e;
}
$unlocked = true;
$secondary = $secondaryUnlock;
});

$this->assertFalse($unlocked, "The lock wasn't acquired");
} finally {
if ($primary) {
$primary();
}
}

yield new Delayed(100 * 2);
$this->assertTrue($unlocked, "The lock wasn't released");

yield $try;
$this->assertInstanceOf("\\Closure", $secondary);
} finally {
if ($secondary) {
$secondary();
}
}
});
}
public function testSharedLock()
{
$this->execute(function () {
$primary = null;
$secondary = null;
try {
$primary = yield $this->tryLock(__FILE__, \LOCK_SH, 100, 100);
$this->assertInstanceOf("\\Closure", $primary);
$secondary = yield $this->tryLock(__FILE__, \LOCK_SH, 100, 100);
$this->assertInstanceOf("\\Closure", $secondary);
} finally {
if ($primary) {
$primary();
}
if ($secondary) {
$secondary();
}
}
});
}
public function testClose()
{
$this->execute(function () {
Expand All @@ -251,7 +324,7 @@ public function testClose()
public function testTruncateToSmallerSize()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");

Expand Down Expand Up @@ -279,7 +352,7 @@ public function testTruncateToSmallerSize()
public function testTruncateToLargerSize()
{
$this->execute(function () {
$path = Fixture::path() . "/write";
$path = Fixture::path()."/write";
/** @var \Amp\File\Handle $handle */
$handle = yield File\open($path, "c+");

Expand Down