Difficulty testing with exception design about promise HOT 17 CLOSED

@dustingraham You are thinking in a sync way. Seem's like you are not considering the real purpose of promises.

Look at this sync code:

try {
     $result = someSyncOperation(); // will block and return when it's complete
} catch (\Exception $exception) {
    echo "Error while doing Operation!";
    $result = false;
}

// $result here sure will be the return value of someSyncOperation() or false on error.
// Just because it's sync!

Now consider an async version like this:

$result = null;

someAsyncOperation() // if it's a true async function, will return immediately.
    ->then(function ($value) use (&$result) {
        $result = $value;

        // $result here sure will be the return value of someAsyncOperation().
        // But at this point could be a future time.
    })
    ->catch(function (\Exception $exception) {
        // Here the operation or what happened inside then() failed for sure.
    });

// $result here probabily still will be null!
// Because everything above just returned immediately.

When you try to write a test in a sync way with PHPUnit real results will be wasted and exceptions will be thrown to the limbo, just because your test method will return before.

Your workaround in second code would only in a few cases, the ones where $promise holds a false async operation (a promise that resolves or rejects synchronously).

from promise.

Closing for now, but I may ping you depending on how implementation goes. Working on it now. Still wrapping my mind around the flow between the two layers of promises.

Thanks.

from promise.

I think catching whatever thrown inside then() is a fundamental promise concept. #46 is not about that, it's about enforce what could be the value of a rejected promise.

Your first code seems like a mistake. The assert will do its normal job by fail and throw, but you aren't listening for any rejections.

I think it's just a problem with how you are testing async stuff, none with #46.

from promise.

@daltones Okay, I understand what you mean about #46 as I was mostly reacting to the subject line. I was coming here to open this issue, and finding the subject line almost contradicted what I was coming here to suggest, it was disheartening. "Enforce \Exception instances as rejection values" when I want to suggest removing \Exception instances as rejection values.

I'm not sure I understand your feedback on the first code. I think the first code is much cleaner and makes perfect sense compared with the second code block.

I build a promise. The promise resolves successfully. Inside of the successful resolution, I want to assert that the successful resolution returned what is expected. Assert that the results are correct.

PHPUnit throws an exception when the assert fails, but then PHPUnit doesn't actually fail the test because the exception is caught by the promise's then() try/catch block.

The try/catch should be left to the developers using the promise library. If I expect an exception inside of the then function, I can ->then(function($results) { try { someDangerousUsageOfResults($results) } catch { /* oh well */ }

The more I think about it, it really makes no sense to try/catch the resolve and push it over to a reject.

from promise.

@dustingraham your third code is actually what you have to avoid when working with promises and async stuff. The point is on chaining ->then()s and ->catch()s so your code just look like normal try/catch, but you don't write it.

from promise.

You can use done() at the end of the promise chain (which btw you should always do when not returning a promise for further consumption):

$promise
    ->then(function($results) {
        $this->assertCount(2, $results);
    })
    ->done();

done() throws unhandled rejection values as exceptions. This is one of the reasons we consider enforcing exceptions as rejection values in #46. done() can then always throw the rejection value instead of wrapping non-exception in a UnhandledRejectionException.

But this does not really tackle the problem that PHPUnit isn't really suited for testing async code because it always assumes synchronous execution of the test methods.

In JavaScript land, test frameworks offer tools for this.

Pseudo code:

test(function(done) {
    expect(1); // Expect 1 assertion
    asyncOperation()
        .then(function(result) {
            assert(result);
        })
        .then(done) // Notify that test is done
});

Additionally you can configure timeouts for tests, that means, if the test does not call done() before the timeout, the test fails.

Another option is, to use @clue's php-block-react#await:

$rowcount = Block\await($promise, $loop);
$this->assertEquals(2, $rowcount);

from promise.

@daltones

There may have been a misunderstanding. I'm not suggesting synchronous code for the asynchronous processing. Your second code block is what I agree with. However, one part of your statement which I disagree.

// Here the operation or what happened inside then() failed for sure.

The bolded part is what this entire issue is about.

The current function names are $promise->then()->otherwise() or $promise->then($successCB, $failedCB). With these function names, I don't expect $successCB to be a try/catch that calls $failedCB with the exception, and prevents the exception from bubbling up.

As a developer, if I write some code that throws an exception, and I do not try/catch the exception. I should end up with some code that crashes.

With the current promise implementation this is not true. Inside the ->then() success callback, if I, as a developer, screw up. The problem is silently ignored. This is what I'm concerned about.

public function testAssertCase()
{
    $deferred = new Deferred();
    $promise = $deferred->promise();
    $promise->then(function($result) {
        throw new \Exception();
    });

    $this->setExpectedException('\Exception');
    $deferred->resolve();
}

This should crash my program completely when I call $deferred->resolve(); But, it doesn't.

@jsor

Thanks! Adding ->done(); after the ->then() call does cause the above test to work. However, this does not work when I am trying to test nested promises.

My nested promise does something like this...

$pool->getConnection()->then(function($connection) {
    $connection->execute()->then(function($result) {
        // handle result
    })->always(function() use ($connection) {
        $pool->releaseConnection($connection);
    })->done();
})->done();

My original code did not have ->done() in there, so I tried adding it. However, the exception that occurs in the // handle result, such as a bad sql query or something, is caught by the try/catch around the resolve call. It bubbles the exception to reject, but then silently disappears.

from promise.

The last code block should throw the exception. Not sure why it does not, maybe a little bit more context is needed (the code block itself could be nested in another promise which swallows the rejection).

It bubbles the exception to reject, but then silently disappears.

Not sure what you mean, i see no reject() in the code.

from promise.

@jsor
Yes, sorry. I tried to generalize it, but the context may have had the problem. I've isolated it to a unit test now.

public function testSilentFail()
{
    // Problem with nested promises.
    $dQuery = new Deferred();
    $dResult = new Deferred();

    $dQuery->promise()
        ->then(function($result) use ($dResult) {
            // Async Task Finished
            $dResult->resolve('Success');
        })
        ->otherwise(function($problem) use ($dResult) {
            // Oops, seems there is a problem.
            $dResult->reject($problem); // <--- THIS dies quietly.
            // This is where the problem is. Reject immediately returns
            // because $dResult promise already has a $this->result value.
        })->done();

    $dResult->promise()
        ->then(function($result) {
            // Handle the result. Oops... programmer bug.
            throw new \Exception('Programmer bug.');
        })->done();

    $this->setExpectedException('\Exception');

    $dQuery->resolve('Async Query is Complete');
}

I dug into this a little more with a debugger. Here is where it appears the chain is dying.

https://github.com/reactphp/promise/blob/master/src/Promise.php#L138

Perhaps the fix is if ->then() yields an exception and does not complete successfully, the promise class should purge the promise result? Thoughts?

from promise.

@dustingraham

This should crash my program completely when I call $deferred->resolve(); But, it doesn't.

No, it won't crash throwing the exception and this is the expected behavior. Just look what you did:

$promise = $deferred->promise();
$promise->then(function($result) {
    throw new \Exception();
});

The second statement actually returns another promise. And this new one will reject, and then you can catch the exception thrown somewhere else.

Imagine this:

$promise = $deferred->promise();
$promise2 = $promise->then(function($result) {
    throw new \Exception();
});

...

$promise2->otherwise(function (\Exception $reason) {
    // Here's the thrown exception!
});

Maybe this is not what you actually want in your code, but to me is pretty right behavior.

from promise.

@daltones Thanks for your response. I added another response to jsor with a unit test that shows the problem. You're right about promise2 and the thrown exception. This makes sense and is logical.

However, when promise 2, with the thrown exception, tries to resolve the nested Deferred, it silently dies because it thinks that it already resolved.

Check out that test case.

from promise.

Btw I personally consider done() kind of dangerous. Your testSilentFail() method is a example of how an unhandled rejection in done() throws the exception elsewhere messing things up.

from promise.

Here's what is happening:

public function testSilentFail()
{
    // Problem with nested promises.
    $dQuery = new Deferred();
    $dResult = new Deferred();

    $dQuery->promise()
        ->then(function($result) use ($dResult) {
            // Async Task Finished
            $dResult->resolve('Success'); // <---- 2. BUG EXCEPTION bubbled here (OMG!)
        })
        ->otherwise(function($problem) use ($dResult) {
            // 3. BUG EXCEPTION was catched here as $problem
            // But $dResult was already resolved above.

            // Oops, seems there is a problem.
            $dResult->reject($problem); // <--- THIS dies quietly.
            // This is where the problem is. Reject immediately returns
            // because $dResult promise already has a $this->result value.
        })->done();

    $dResult->promise()
        ->then(function($result) {
            // Handle the result. Oops... programmer bug.
            throw new \Exception('Programmer bug.'); // <---- 1. BUG EXCEPTION created and thrown here
        })->done();

    $this->setExpectedException('\Exception');

    $dQuery->resolve('Async Query is Complete');
}

As you can see, done() as a promise consumer is crashing the promise resolver. That's why for me it's not a good idea.

from promise.

When I remove ->done() it still fails to pass the \Exception back out to $dQuery->resolve(...) so there is no way to try/catch $dQuery->resolve('Async Query is Complete');

If a developer using this library creates a bug on the line above where the new exception is thrown, the promise library is currently quietly terminating in the above use case.

from promise.

First, promises are immutable. This means, once a promise is resolved (fulfilled or rejected), its state cannot be changed.

I reordered your example code to better illustrate the flow:

$dQuery = new Deferred();
$dResult = new Deferred();

$dQuery->promise()
    ->then(function($result) use ($dResult) {
        $dResult->promise()
            ->then(function($result) {
                // Handle the result. Oops... programmer bug.
                throw new \Exception('Programmer bug.');
            })->done();

        $dResult->resolve('Success');
    })
    ->otherwise(function($problem) use ($dResult) {
        // $problem is `\Exception('Programmer bug.')` here!

        // The following is a no-op because the promise of $dResult is already
        // resolved in the previous then()!
        $dResult->reject($problem);

        // Because you do not "rethrow" $problem here, the rejection is
        // considered as "catched".
        // You have to `throw $problem` here or `return React\Promise\reject($problem)`.
    })->done();

$this->setExpectedException('\Exception');

$dQuery->resolve('Async Query is Complete');

from promise.

@jsor Ok... I see what you're saying.

It is a little tricky because the $dQueryPromise->otherwise() call can be triggered on either a failed $dQuery->reject() or an exception that occurs in $dQueryPromise->then()

This entire thread is a non-issue if only dealing with one or the other. But, when it can be either of those, and I must handle either a $problem value that stems from the dQuery async execution, or a $problem that stems from the success callback of that execution, it gets complicated.

Since the entire point of this thread is that a developer can make a mistake in that success callback, I think what I'll do is create a QueryException and in the otherwise callback, if $problem is not a QueryException then re-throw it. And, that'll solve this use case. This way if any other exception occurs inside of $dQuery->promise()->then(function($result) { /* here */ }) the otherwise block will re-throw the problem.

I think I'm coming around to the idea here. The longer I stare at your re-written example, the more I feel like it makes sense that $dResult->reject($problem) can't be triggered again. But, hopefully you see why it is confusing for me. If ->otherwise() is triggered from a failed async call, then reject is called without any problem. But, if ->otherwise() is triggered from a failed ->then() callback, the reject is not called. Understanding this now though, I will try to work around it.

Appreciate your patience.

from promise.

You're welcome :) Feel free to post/link actual code here if you need help somewhere.

from promise.