Skip to content

Commit

Permalink
Fixing nested waits & cannot resolve > once
Browse files Browse the repository at this point in the history 
This commit updates promises so that they can only be resolved or
rejected once, and instead of ignoring subsequent attempts, throws an
exception. This is to make it easier to spot bugs related to promise
resolution. This commit updates the wait function to wait on a result
until a value is returned, including waiting on any promises (i.e., if
you're waiting on a then promise that returns a promise, then the wait
function will also wait on the forwarded promise).
  • Loading branch information
@mtdowling
mtdowling committed Mar 8, 2015
1 parent 085c076 commit e314897
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 62 deletions.
84 changes: 26 additions & 58 deletions src/Promise.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ class Promise implements PromiseInterface
/** @var array[] Array of [promise, fulfilled, rejected] */
private $handlers = [];

/** @var callable[] Dependent wait functions */
private $waitFns;
/** @var callable Wait function */
private $waitFn;

/** @var callable */
private $cancelFn;
Expand Down Expand Up @@ -52,7 +52,7 @@ public function __construct(
callable $waitFn = null,
callable $cancelFn = null
) {
$this->waitFns = $waitFn ? [$waitFn] : [];
$this->waitFn = $waitFn;
$this->cancelFn = $cancelFn;
}

Expand All @@ -61,19 +61,7 @@ public function then(
callable $onRejected = null
) {
if ($this->state === 'pending') {
$p = new Promise(
function () {
// Provide a wait function to the dependent promise that
// allows the promise to wait on parent promises. Note
// that this connection is severed once the promise has
// been delivered.
/** @var callable $wfn */
while ($wfn = array_shift($this->waitFns)) {
$wfn();
}
},
$this->cancelFn
);
$p = new Promise([$this, 'wait'], [$this, 'cancel']);
// Keep track of this dependent promise so that we resolve it
// later when a value has been delivered.
$this->handlers[] = [$p, $onFulfilled, $onRejected];
Expand All @@ -99,14 +87,16 @@ function () {
public function wait($unwrap = true, $defaultDelivery = null)
{
if ($this->state === 'pending') {
if (!$this->waitFns) {
if (!$this->waitFn) {
// If there's not wait function, then resolve the promise with
// the provided $defaultDelivery value.
$this->resolve($defaultDelivery);
} else {
try {
// Invoke the wait fn and ensure it resolves the promise.
call_user_func(array_shift($this->waitFns));
$wfn = $this->waitFn;
$this->waitFn = null;
$wfn();
if ($this->state === 'pending') {
throw new \LogicException('Invoking the wait callback did not resolve the promise');
}
Expand All @@ -122,14 +112,20 @@ public function wait($unwrap = true, $defaultDelivery = null)
return null;
}

// Wait on nested promises until a normal value is unwrapped/thrown.
$result = $this->result;
while ($result instanceof PromiseInterface) {

This comment has been minimized.

Sign in to view

Copy link
@jeremeamia

jeremeamia Mar 9, 2015

Member

Ah ha, that's where we wanted the loop.
$result = $result->wait();
}

if ($this->state === 'fulfilled') {
return $this->result;
return $result;
}

// It's rejected or cancelled, so "unwrap" and throw an exception.
throw $this->result instanceof \Exception
? $this->result
: new RejectionException($this->result);
throw $result instanceof \Exception
? $result
: new RejectionException($result);
}

public function getState()
Expand All @@ -143,7 +139,7 @@ public function cancel()
return;
}

$this->waitFns = [];
$this->waitFn = null;

if ($this->cancelFn) {
$fn = $this->cancelFn;
Expand All @@ -163,13 +159,12 @@ public function cancel()
public function resolve($value)
{
if ($this->state !== 'pending') {
return;
throw new \RuntimeException("Cannot resolve a {$this->state} promise");
}

$this->state = 'fulfilled';
$this->result = $value;
$this->cancelFn = null;
$this->waitFns = [];
$this->cancelFn = $this->waitFn = null;

if ($this->handlers) {
$this->deliver($value);
Expand All @@ -179,13 +174,12 @@ public function resolve($value)
public function reject($reason)
{
if ($this->state !== 'pending') {
return;
throw new \RuntimeException("Cannot reject a {$this->state} promise");
}

$this->state = 'rejected';
$this->result = $reason;
$this->cancelFn = null;
$this->waitFns = [];
$this->cancelFn = $this->waitFn = null;

if ($this->handlers) {
$this->deliver($reason);
Expand Down Expand Up @@ -328,7 +322,9 @@ private function resolveForwardPromise(array $group)

switch ($promise->getState()) {
case 'pending':
$this->resolvePendingPromise($promise, $handlers);
// The promise is an instance of Promise, so merge in the
// dependent handlers into the promise.
$promise->handlers = array_merge($promise->handlers, $handlers);
return null;
case 'fulfilled':
return [
Expand All @@ -347,34 +343,6 @@ private function resolveForwardPromise(array $group)
return null;
}

/**
* The promise is pending, resolve the remaining handlers after it.
*
* @param Promise $promise Forwarded promise.
* @param array $handlers Dependent handlers.
*/
private function resolvePendingPromise(Promise $promise, array $handlers)
{
// The promise is an instance of Promise, so merge in the dependent
// handlers into the promise.
$promise->handlers = array_merge($promise->handlers, $handlers);
$waiter = [$promise, 'wait'];
$this->waitFns[] = $waiter;

// When the promise resolves, remove the associated pending waitFn
// from this promise to allow variables to be garbage collected.
$removeCb = function () use ($waiter) {
foreach ($this->waitFns as $i => $w) {
if ($w === $waiter) {
unset($this->waitFns[$i]);
break;
}
}
};

$promise->then($removeCb, $removeCb);
}

/**
* Resolve the dependent handlers recursively when the promise resolves.
*
Expand Down
39 changes: 35 additions & 4 deletions tests/PromiseTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ public function testReturnsPromiseForThennable()
$this->assertSame($p, Promise::promiseFor($p));
}

/**
* @expectedException \RuntimeException
* @expectedExceptionMessage Cannot resolve a fulfilled promise
*/
public function testCannotResolveNonPendingPromise()
{
$p = new Promise();
Expand All @@ -35,6 +39,10 @@ public function testCannotResolveNonPendingPromise()
$this->assertEquals('foo', $p->wait());
}

/**
* @expectedException \RuntimeException
* @expectedExceptionMessage Cannot reject a fulfilled promise
*/
public function testCannotRejectNonPendingPromise()
{
$p = new Promise();
Expand Down Expand Up @@ -105,6 +113,14 @@ public function testRejectsSelfWhenWaitThrows()
}
}

public function testWaitsOnNestedPromises()
{
$p = new Promise(function () use (&$p) { $p->resolve('_'); });
$p2 = new Promise(function () use (&$p2) { $p2->resolve('foo'); });
$p3 = $p->then(function () use ($p2) { return $p2; });
$this->assertSame('foo', $p3->wait());
}

public function testCannotCancelNonPending()
{
$p = new Promise();
Expand Down Expand Up @@ -209,14 +225,26 @@ public function testCreatesPromiseWhenRejectedWithNoCallback()
$this->assertInstanceOf('GuzzleHttp\Promise\RejectedPromise', $p2);
}

public function testStacksThenWaitFunctions()
public function testInvokesWaitFnsForThens()
{
$p = new Promise(function () use (&$p) { $p->resolve('a'); });
$p2 = $p->then(function ($v) { return $v . '-1-'; })
$p2 = $p
->then(function ($v) { return $v . '-1-'; })
->then(function ($v) { return $v . '2'; });
$this->assertEquals('a-1-2', $p2->wait());
}

public function testStacksThenWaitFunctions()
{
$p1 = new Promise(function () use (&$p1) { $p1->resolve('a'); });
$p2 = new Promise(function () use (&$p2) { $p2->resolve('b'); });
$p3 = new Promise(function () use (&$p3) { $p3->resolve('c'); });
$p4 = $p1
->then(function () use ($p2) { return $p2; })
->then(function () use ($p3) { return $p3; });
$this->assertEquals('c', $p4->wait());
}

public function testForwardsFulfilledDownChainBetweenGaps()
{
$p = new Promise();
Expand Down Expand Up @@ -283,9 +311,12 @@ public function testForwardsHandlersToNextPromise()
{
$p = new Promise();
$p2 = new Promise();
$p->then(function ($v) use ($p2) { return $p2; });
$resolved = null;
$p->then(function ($v) use ($p2) { return $p2; })
->then(function ($value) use (&$resolved) { $resolved = $value; });
$p->resolve('a');
$p->resolve('b');
$p2->resolve('b');
$this->assertEquals('b', $resolved);
}

public function testRemovesReferenceFromChildWhenParentWaitedUpon()
Expand Down

0 comments on commit e314897

Please sign in to comment.