Queue broken when throwing from done() (dev-master only)

[kelunik]

In case there's something enqueued that throws, the queue is never drained again if it wasn't empty after throwing.

<?php

require __DIR__ . "/vendor/autoload.php";

use React\Promise\FulfilledPromise;
use React\Promise\RejectedPromise;

$promiseA = new RejectedPromise(new RuntimeException("A"));

try {
    $promiseA->done(null, function ($e) {
        $promiseB = new RejectedPromise(new RuntimeException("B"));
        $promiseB->then(null, function () {
            print "OK" . PHP_EOL;
        });

        throw $e;
    });
} catch (Throwable $e) {
    // exception caught
}

$promiseC = new FulfilledPromise("C");
$promiseC->then(function () {
    // never executed

    print "C" . PHP_EOL;
});

print " -- END -- " . PHP_EOL;

It's a specially crafted example, not sure whether it will ever happen accidentally. #97 mostly solves this, but should probably also trigger fatal errors thrown from callbacks called in Queue::drain().

[jsor]

Thanks for reporting. I've already noticed that myself, here's a patch that fixes that: master...jsor-labs:fix-queue

The alternative approach is #97 but you're probably right, we should consider handling exceptions thrown from drain() (though, theoretically that should never happen).

[kelunik]

The proposed solution would just hide a second exception if there were two, no? It's probably best to just not rethrow but to use fatalError from #97.

[jsor]

Yes, that was just a prototype. I stopped working on it in favor of #97.

[clue]

I'm not sure I'm of much help here, as I'm not sure I follow, please help me understand and document this from a consumer's perspective. This ticket makes it sound like there's some major bug in this library, but as far as I can tell the issue you're seeing (or rather contriving?) is that you're throwing from the done() method which is not allowed as per the documentation?

I'm not saying that this situation couldn't be improved, but I think we should spend the time to help communicate this issue first. The title "Queue broken after one exception" is clearly misleading here (also, what "queue"?), may I ask either of you guys to change this to reflect what is actually going on here? Thank you!

[kelunik]

is that you're throwing from the done() method which is not allowed as per the documentation?

While not being allowed, it might happen. And if it happens, then the entire promise library is broken.

This ticket makes it sound like there's some major bug in this library

Yes, it is, but as it's not released yet, it doesn't matter too much for now.

also, what "queue"?

The only one that exists, it's only in master: https://github.com/reactphp/promise/blob/master/src/Internal/Queue.php

[clue]

I believe this has been answered and resolved via #97, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍