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.

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 timeoutWithDefault function #245

Merged
merged 4 commits into from
Nov 25, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
41 changes: 41 additions & 0 deletions lib/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,47 @@ function timeout($promise, int $timeout): Promise
return $deferred->promise();
}

/**
* Creates an artificial timeout for any `Promise`.
*
* If the promise is resolved before the timeout expires, the result is returned
*
* If the timeout expires before the promise is resolved, a default value is returned
*
* @param \Amp\Promise|\React\Promise\PromiseInterface $promise Promise to which the timeout is applied.
* @param int $timeout Timeout in milliseconds.
* @param mixed $default
*
* @return \Amp\Promise
*
* @throws \TypeError If $promise is not an instance of \Amp\Promise or \React\Promise\PromiseInterface.
*/
function timeoutWithDefault($promise, int $timeout, $default = null): Promise
{
if (!$promise instanceof Promise) {
if ($promise instanceof ReactPromise) {
$promise = adapt($promise);
} else {
throw createTypeError([Promise::class, ReactPromise::class], $promise);
}
}

$promise = timeout($promise, $timeout);

$deferred = new Deferred();
$newPromise = $deferred->promise();

$promise->onResolve(function ($exception, $value) use ($deferred, $default) {
if ($exception) {
$deferred->resolve($default);
Copy link
Member

Choose a reason for hiding this comment

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

An exception should probably be thrown here instead of resulting in the default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, give me a minute.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't make use of the timeout function, but rather adapt the code of it to suit this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No wait, that's the whole purpose of the default value. You timeout with a default and when you'll receive a timeout exception, you'll get the default value.

Copy link
Member

Choose a reason for hiding this comment

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

You'll get the default on a timeout, but not if the promise fails?

} else {
$deferred->resolve($value);
}
});

return $newPromise;
}

/**
* Adapts any object with a done(callable $onFulfilled, callable $onRejected) or then(callable $onFulfilled,
* callable $onRejected) method to a promise usable by components depending on placeholders implementing
Expand Down
129 changes: 129 additions & 0 deletions test/TimeoutWithDefaultTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
<?php

namespace Amp\Test;

use Amp\Delayed;
use Amp\Failure;
use Amp\Loop;
use Amp\Promise;
use Amp\Success;
use function React\Promise\resolve;

class TimeoutWithDefaultTest extends \PHPUnit\Framework\TestCase
{
public function testSuccessfulPromise()
{
Loop::run(function () {
$value = 1;

$promise = new Success($value);

$promise = Promise\timeoutWithDefault($promise, 100, 2);
$this->assertInstanceOf(Promise::class, $promise);

$callback = function ($exception, $value) use (&$result) {
$result = $value;
};

$promise->onResolve($callback);

$this->assertSame($value, $result);
});
}

public function testFailedPromise()
{
Loop::run(function () {
$exception = new \Exception;
$expected = 2;

$promise = new Failure($exception);

$promise = Promise\timeoutWithDefault($promise, 100, $expected);
$this->assertInstanceOf(Promise::class, $promise);

$callback = function ($exception, $value) use (&$actual) {
$actual = $value;
};

$promise->onResolve($callback);

$this->assertEquals($expected, $actual);
});
}

/**
* @depends testSuccessfulPromise
*/
public function testFastPending()
{
$value = 1;

Loop::run(function () use (&$result, $value) {
$promise = new Delayed(50, $value);

$promise = Promise\timeoutWithDefault($promise, 100);
$this->assertInstanceOf(Promise::class, $promise);

$callback = function ($exception, $value) use (&$result) {
$result = $value;
};

$promise->onResolve($callback);
});

$this->assertSame($value, $result);
}

/**
* @depends testSuccessfulPromise
*/
public function testSlowPending()
{
$expected = 2;

Loop::run(function () use (&$actual, $expected) {
$promise = new Delayed(200);

$promise = Promise\timeoutWithDefault($promise, 100, $expected);
$this->assertInstanceOf(Promise::class, $promise);

$callback = function ($exception, $value) use (&$actual) {
$actual = $value;
};

$promise->onResolve($callback);
});

$this->assertEquals($expected, $actual);
}

/**
* @depends testSuccessfulPromise
*/
public function testReactPromise()
{
Loop::run(function () {
$value = 1;

$promise = resolve($value);

$promise = Promise\timeoutWithDefault($promise, 100, 2);
$this->assertInstanceOf(Promise::class, $promise);

$callback = function ($exception, $value) use (&$result) {
$result = $value;
};

$promise->onResolve($callback);

$this->assertSame($value, $result);
});
}

public function testNonPromise()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@trowski I tested your suggested implementation and then this test fails, because the TypeError is not thrown.

Copy link
Member

Choose a reason for hiding this comment

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

It should be…

Copy link
Member

@trowski trowski Nov 3, 2018

Choose a reason for hiding this comment

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

Oh, right, because it's not resolving the promise with the TypeError. Fine, then it needs to be this way:

function timeoutWithDefault($promise, int $timeout, $default = null): Promise
{
    $promise = timeout($promise, $timeout);

    return call(function () use ($promise, $default) {
        try {
            return yield $promise;
        } catch (TimeoutException $exception) {
            return $default;
        }
    });
}

{
$this->expectException(\TypeError::class);
Promise\timeoutWithDefault(42, 42);
}
}