Enforce \Exception instances as rejection values about promise HOT 28 CLOSED

+1 👍

from promise.

👍

from promise.

Note that some() currently rejects with an array of promises. Maybe we need some kind of composite exception class.

from promise.

Good catch, thanks 👍

from promise.

I think you should allow \Throwable in PHP7.

public function __construct($e)
{
  if (PHP_MAJOR_VERSION < 7) {
    if (!$e instanceof \Exception) {
      throw new \InvalidArgumentException;
    }
  } elseif (!$e instanceof \Throwable) {
    throw new \InvalidArgumentException;
  }
}

from promise.

Another good catch 👍

from promise.

Creating exceptions is very heavy for PHP due to filling them with data on creation and not on throwing. Just check what happens when you create 10k of promises, and reject them all. In best sitatuion you get MemoryOverflow error, in worst core dumped. It is reasonable to allow rejection with different values than Throwable for this purpose. I think the good idea would be to allow string or Throwable or some lazy Throwable object defined inside library.

from promise.

@joshdifabio

Note that some() currently rejects with an array of promises. Maybe we need some kind of composite exception class.

Maybe we could solve this if some() would reject with the first exception, and not wait for other values? And make a separate function returning a promise that resolves into an array of rejected values.

I think a rejected value should always be something Throwable and there would be no arrays.

from promise.

@khelle

I think you might be mistaking about memory consumption, I ran the following code:

php -r '$e = []; for ($i = 0; $i < 10000; $i++) { $e[] = new Exception("Test"); }; var_dump(memory_get_usage()); '

And I get 4-5Mb of used memory (that is 500 bytes per Exception object). Maybe you were using some xdebug options that add additional information into an exception?

I also tried this code:

use React\Promise\Deferred;

$p = [];
for ($i=0; $i < 10000; $i++) { 
    $p[] = new Deferred();
}

foreach ($p as $deferred) {
    $deferred->reject(new \Exception("Test"));
}

var_dump(memory_get_peak_usage());

It prints 20 Mb peak memory usage (2 Kb per rejected promise). So it is not exceptions that consume memory but something else. Promises create a lot of anonymous functions inside, maybe that could be the reason.

from promise.

@codedokode This is known mistake of PHP that it popullates Exception on creation and not on throwing like in other programming languages, for example in JAVA. There is no dyning that. The example you provided without using exceptions would take about ~10 Mb peak memory, meaning the exception took 10 Mb. It is already high enough for exceptions that literally have empty stack trace. Now try the same example in real world application, where the thrown exception would have about ~10-20 elements in their stack trace. You will run out of memory.

from promise.

@khelle I would call that a design choice rather than mistake. I made measurements with code that emulates a "complex" application:

<?php 

class A1 { public static function test() {  A2::test(); } };
class A2 { public static function test() {  A3::test(); } };
class A3 { public static function test() {  A4::test(); } };
class A4 { public static function test() {  A5::test(); } };
class A5 { public static function test() {  A6::test(); } };
class A6 { public static function test() {  A7::test(); } };
class A7 { public static function test() {  A8::test(); } };
class A8 { public static function test() {  A9::test(); } };
class A9 { public static function test() {  A10::test(); } };
class A10 { public static function test() {  A11::test(); } };
class A11 { public static function test() {  A12::test(); } };
class A12 { public static function test() {  A13::test(); } };
class A13 { public static function test() {  A14::test(); } };
class A14 { public static function test() {  A15::test(); } };
class A15 { public static function test() { 
    throw new \Exception("Test");
} };

$list = [];
for ($i=0; $i < 10000; $i++) { 
    try {
        A1::test();
    } catch (Exception $e) {
        $list[] = $e;
    }
}

var_dump(memory_get_peak_usage());

It shows 132Mb peak usage (13 Kb per exception object) on my machine (32-bit) without xdebug and 152Mb if xdebug is enabled. Maybe this should be reported to PHP developers. Maybe they will want to optimize it.

By the way in PHP7 the exceptions are no longer required to be inherited from Exception class. You can write your own class even without any fields. Or you could reuse objects.

from promise.

We need to address memory leaks before enforcing exceptions as rejection reasons. Consider following example:

new React\Promise\Promise(
    function ($resolve, $reject) {
        $reject('Rejected.');
    }
);

collectGarbage();

new React\Promise\Promise(
    function ($resolve, $reject) {
        $reject(new \RuntimeException('Rejected.'));
    }
);

collectGarbage();

function collectGarbage()
{
    printf(
        "%.3fMB => %d => %.3fMB (%.3fMB)\n",
        memory_get_usage() / 1024 / 1024,
        gc_collect_cycles(),
        memory_get_usage() / 1024 / 1024,
        memory_get_peak_usage() / 1024 / 1024
    );
}

Output:

0.615MB => 0 => 0.615MB (0.659MB)
0.619MB => 27 => 0.615MB (0.659MB)

As you can see, using an exception as rejection reason created 27 garbage objects.

from promise.

@valga These are circular references, not memory leaks.

from promise.

@valga A small elaboration why these circular references are created when rejecting with exceptions:

When creating a Promise, $resolver is called via Promise::call. Promise::call calls that callback with three closures, each having a reference to the promise itself. Now let's take a look at your example:

new React\Promise\Promise(
    function ($resolve, $reject) {
        $reject(new \RuntimeException('Rejected.'));
    }
);

The $resolver receives $resolve and $reject there, having references to the promise as mentioned. It then creates a RuntimeException. Exceptions in PHP contain the entire stack trace including references to the arguments of each function call. That means the exception contains references to $resolve and $reject, which have both a reference to the promise. The promise on the other hand has a reference to the exception (obviously). So there's the circular reference.

There has been a thread on #internals recently regarding exceptions collecting these references by default.

from promise.

Thanks @kelunik for the explanation. 👍

I think, there isn't much we can do about this since we can't throw without referencing arguments (like debug_backtrace()allows with the DEBUG_BACKTRACE_IGNORE_ARGS flag).

FTR, by convention, we already always reject promises throughout all ReactPHP with exceptions.

There is #55 and also #56 discussing cleaning up resources on cancellation, but that would be a BC break and only allowed in the next major version.

from promise.

@jsor Did you think about using CancellationTokens like Amp for cancellation instead of a cancel() method on Promise? I think that makes passing them down easier and solves the problem of not knowing in advance how many interested parties there are (requiring as many cancel() calls as then() calls, which only works if there's no then() after the promise has been cancelled).

Regarding the backtrace: I guess the only think that can be done is using Deferred instead of Promise, because that doesn't have itself in the call trace.

from promise.

I guess the only think that can be done is using Deferred instead of Promise, because that doesn't have itself in the call trace.

Deferred has the same number of circular references on rejection as Promise does. The only way to prevent circular references is to pass a reference to rejector (or deferred) via global scope into nextTick() call, but it is a dirty hack.

Although you could lower a number of circular references by throwing an exception in nextTick() call, because it will have smaller stack trace:

$loop->nextTick(function () use ($reject) {
    $reject(new \RuntimeException('Error'));
});

from promise.

Deferred has the same number of circular references on rejection as Promise does.

How so? The $deferred is not passed as parameter where new \RuntimeException is called and so it doesn't end up being referenced by the exception trace.

Although you could lower a number of circular references by throwing an exception in nextTick() call, because it will have smaller stack trace:

The number of circular references isn't smaller because of a smaller stack trace. But in that case $reject isn't in the call trace anymore, so it solves the problem. However, you loose all the stack trace information.

from promise.

How so?

Because you have to write something like use($deferred) to reject it.

But in that case $reject isn't in the call trace anymore, so it solves the problem.

It doesn't solve the problem, because $reject is in use anyway.

from promise.

It doesn't matter whether it's in use. The closure will have a reference to that variable, yes, but it's not in the call trace, and that's what matters.

from promise.

Did you think about using CancellationTokens like Amp for cancellation instead of a cancel() method on Promise?

Yes, i did. I Studied tc39/proposal-cancelable-promises and alike. I tried hard to like it, but I couldn't quite manage it. ;)

I think that makes passing them down easier and solves the problem of not knowing in advance how many interested parties there are (requiring as many cancel() calls as then() calls, which only works if there's no then() after the promise has been cancelled).

In the proposal i made, calls to then() after cancellation returns a immediate rejected promise. This is the same behaviour bluebird has and also Rx observables behave similar by throwing when subscribing to a disposed observable (IIRC).

from promise.

What makes you feel uneasy with these tokens?

In the proposal i made, calls to then() after cancellation returns a immediate rejected promise.

That's probably the only reasonable choice with that API, because the future can't be known and cancellation can't wait potential future handlers being attached.

from promise.

What makes you feel uneasy with these tokens?

I just find the API not very intuitive.

from promise.

The closure will have a reference to that variable, yes, but it's not in the call trace, and that's what matters.

If the closure is in stack trace, and the closure has a reference to that variable, stack trace also contains a reference to that variable.

Consider 2 examples:

$loop = React\EventLoop\Factory::create();
new \React\Promise\Promise(function ($resolve, $reject) use ($loop) {
    $loop->nextTick(function () use ($reject) {
        $reject(debug_backtrace());
    });
});
$loop->run();
var_dump(gc_collect_cycles());

int(24)

$loop = React\EventLoop\Factory::create();
new \React\Promise\Promise(function ($resolve, $reject) use ($loop) {
    $loop->nextTick(function () use ($reject) {
        $reject(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS));
    });
});
$loop->run();
var_dump(gc_collect_cycles());

int(0)

from promise.

I guess you're right, forgot that closures are also just objects. You can use that:

<?php

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

$loop = React\EventLoop\Factory::create();

new \React\Promise\Promise(function ($resolve, $reject) use ($loop) {
    $loop->nextTick(function () use (&$reject) {
        $r = $reject;
        $reject = null;
        $r(debug_backtrace());
    });
});

$loop->run();

var_dump(gc_collect_cycles());

from promise.

@kelunik Very nice catch! This does indeed prevent these arguments from showing up in the call stack for PHP 7+ and HHVM, here's a gist to reproduce this https://3v4l.org/OiDr4

from promise.

@valga We need to address memory leaks before enforcing exceptions as rejection reasons. Consider following example […]

Thank you for looking into this! I agree that this is something we absolutely want to look in to!

Thanks to the above analysis, I think we located some of the issues why the above code does indeed create some cyclic garbage references. Let's not call this a "memory leak", because memory was eventually freed, but this clearly caused some unexpected and significant memory growth.

@clue Creating a new Exception […] will store a reference to the stack trace including the full call stack with all arguments passed. Because the $resolver and $canceller are always invoked with three callable arguments, these get implicitly stored as part of this stack. Because each of these arguments is a callable that is implicitly bound to the promise, this hence creates a cyclic reference. Note that these parameters are part of the call stack because they're passed at call time, despite not being used anywhere within this callback.

By explicitly assigning these arguments with null values, we can ensure that these null values end up in the call stack instead, effectively preventing these cyclic references. This allows PHP to rely on its refcount to immediate free these variables without having to rely on cyclic garbage collection kicking in at a later time after accumulating some memory.

reactphp/socket#159 (comment)

I've just filed #113 which improves this for the somewhat common use case of simply throwing an Exception without ever accessing the $resolve and $reject arguments at all. Empirical evidence suggests that this is a rather common use case.

In a follow-up PR we'll look into avoiding cyclic garbage references when these arguments are actually used. This can be achieved by explicitly binding the closure to another instance and then passing references variables to them. This does require some significant testing effort.

In the meantime, this can also be worked around by explicitly storing a reference to $reject and then assigning $resolve = $reject = $notify = null; (for PHP 7+ only) as documented in the previous comment by @kelunik and (temporarily) implemented in reactphp/socket#159.

from promise.

This discussion got a bit sidetracked due to possible memory issues when using Exceptions, so I'm trying to wrap this up.

All such memory issues have been addressed via the referenced tickets that have just been released via https://github.com/reactphp/promise/releases/tag/v2.6.0. Thank you all for participating in this discussion and bringing up some very good pointers!

This means that there seem to be no other open issues with regards to enforcing Exceptions.

from promise.