weak handles and promises in progress stuff

[Fishrock123]

Important: Please do not discuss the merits of this here. I will make the case for it once it works. This will be put into a new PR once ready. For now, I am only interested in figuring out why this does not work.

---

I'm not sure where to ask this with the full details, so I guess here will work.

This attempts to make a GC handler for unhandled promise rejections, but currently the weak handle appears to make the promise un-GC-able. I worked with @trevnorris on this and his conclusion was also that it is probably a V8 bug, but I'd like more review on the method.

This problem persists when applied onto the vee-eight-4.9 branch.

Here's the test failure showing it with this patch:

=== release test-promises-unhandled-rejections ===                             
Path: parallel/test-promises-unhandled-rejections
'synchronously rejected promise should trigger unhandledRejection' failed

Error: Async test timeout exceeded
    at null._repeat (/Users/Jeremiah/Documents/node/test/parallel/test-promises-unhandled-rejections.js:58:23)
    at wrapper [as _onTimeout] (timers.js:284:11)
    at Timer.listOnTimeout (timers.js:92:15)
From previous event:
    at asyncTest (/Users/Jeremiah/Documents/node/test/parallel/test-promises-unhandled-rejections.js:45:17)
    at Object.<anonymous> (/Users/Jeremiah/Documents/node/test/parallel/test-promises-unhandled-rejections.js:109:1)
    at Module._compile (module.js:417:34)
    at Object.Module._extensions..js (module.js:426:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:451:10)
    at startup (node.js:153:18)
    at node.js:963:3

cc @nodejs/v8

[ofrobots]

Here's a reduced version of your testcase.

process.on('unhandledRejection', function(reason, promise) {
  console.log(`got unhandled rejection at ${promise} with reason ${reason}`);
});
Promise.reject(new Error('foo'));

GC doesn't give you a guarantee on when the weak callback will be called. Here's the comment about weak callbacks (c.f. v8.h):

/**
   *  Install a finalization callback on this object.
   *  NOTE: There is no guarantee as to *when* or even *if* the callback is
   *  invoked. The invocation is performed solely on a best effort basis.
   *  As always, GC-based finalization should *not* be relied upon for any
   *  critical form of resource management!
   */

I think what is happening in your case is that the event loop drains and node terminates before GC has had a chance to run and call your weak callbacks.

To verify this theory, I tried the following test-case:

process.on('unhandledRejection', function(reason, promise) {
  console.log(`got unhandled rejection at ${promise} with reason ${reason}`);
});
Promise.reject(new Error('foo'));
setTimeout(function() {
  gc(); gc(); gc();
}, 1000);

There are some bugs in your patch, but I think this works better.
EDIT: run the latter test case with --expose-gc

[Fishrock123]

Hmmm, that is perhaps too unreliable for the user event (I was thinking of wrapping it in an immediate so that it was not directly fired off of GC), but could still be used for our own warning / error purposes.

@ofrobots I realize this is probably not implemented in V8, but is it possible for a JS vm to detect when an object has gone out of scope/has no references rather than waiting for when GC is needed? Or is that expensive enough that it is wrapped into GC?

[ofrobots]

I think it should be possible to implement a vm with a reference counting GC that could give you immediate finalization. This would be slower however. Modern GC algorithms tend to optimize for peak performance rather than immediacy of object finalization.

[trevnorris]

I'm not following why "track-promise.h" isn't included here.

[Fishrock123]

@trevnorris Actually, it seemed to function without anything being included. I really don't know why?

[ofrobots]

It seems you have a copy of the TrackPromise class duplicated in this file below – that's why it works.

If nothing else, other than this files, needs to know about the TrackPromise class, you don't need track-promise.h.

[trevnorris]

@ofrobots Was it explained that if the Promise was made Persistent immediately then allowed to leave scope, it would eventually be GC'd. But as soon as it was rejected the WeakCallback would no longer fire. Even though the rejected Promise was in fact being cleaned up (verified this by only making rejected Promises en mass, and not seeing memory usage increase despite the weak callbacks never firing).

[ofrobots]

@trevnorris Do you already have a test-case for the 'making rejected Promises en mass' scenario that I can play with?

[Fishrock123]

@ofrobots I'm pretty (90%) sure he was just doing this:

(Or at least that is what he posted to me in slack like 2? weeks ago hahaha)

'use strict';

(function runner(n) {
  if (--n <= 0)
    return;
  new Promise(function(res, rej) { rej({}) });
  process.nextTick(runner, n);
}(1e5));

[vkurchatkin]

FWIW it works using node-weak:

var weak = require('weak')

process.on('unhandledRejection', function(reason, promise) {
  weak(promise, () => console.log('gc'));
});


Promise.reject(new Error('foo'));

setTimeout(() => gc(), 1000);

[ofrobots]

@Fishrock123 @trevnorris In that test-case if I change process.nextTick to setImmediate I do see the weak callbacks getting fired.

[Fishrock123]

@ofrobots / @trevnorris I've updated the code to only try to a default action on GC, but I'm getting a fatal error:

(My terminal is fish, if you are curious.)

../node/node --expose-gc promise-error.js 
hello
this is a promise!
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
fish: '../node/node --expose-gc promis…' terminated by signal SIGABRT (Abort)

Using this test case:

try {
  console.log('hello')
  new Promise(function(res, rej) {
    console.log('this is a promise!')
    consol.log('oops')
  });
} catch (err) {
  console.error('caught error:', err)
}

// do some stuff, spend some time, idk
var i = 5
var timeout = setInterval(() => {
  gc()
  var what = { i }
  if (i-- === 0) {
    clearTimeout(timeout)
  }
  console.log(what.i)
}, 200)

[Fishrock123]

Looks that error is from here, am I doing something incorrectly?

[ofrobots]

You can drop the .ToLocalChecked(). I don't think you need it.

[Fishrock123]

Hmm, that gives me this warning:

[38/39] CXX obj/src/node.track-promise.o
../../src/track-promise.cc:67:5: warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]
    fn->Call(env->context(), Null(env->isolate()), 1, &promise);
    ^~~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[ofrobots]

Yes, you can ignore for now, but eventually you will need to decide whether you need to do anything with the value that the fn returns. The reason ToLocalChecked doesn't work is because that function asserts that the Local be non-empty. Maybe look at using FromMaybe instead.

Alternatively you can ensure that fn always returns non-empty.

[bnoordhuis]

The return value is an empty handle when the JS function throws an exception so .ToLocalChecked() is almost always wrong for function calls.

Aside, I'm not sure if it's allowed to call into the VM from inside a weak callback. The VM state is EXTERNAL at that point so it's probably okay, but...

[ofrobots]

That's a good point. I think the original intent of fn is to conditionally abort the process here. It should be do-able in C++.

Also note that with the new style weakness (which becomes the only non-deprecated option with V8 4.9), the promise object is already be GC'd by the time the weak callback is called and you will not have a handle to it.

[Fishrock123]

Right, I'm trying to move it to C++ but I'm not sure how I can iterate over a Local<Array> in C++, so I'm going to try doing that in JS for now...

[trevnorris]

Here's as slightly lengthy native module for testing:

#include <v8.h>
#include <node.h>

using namespace v8;

class CallbackInfo {
 public:
  static inline CallbackInfo* New(Isolate* isolate, Local<Promise> promise);
 private:
  static void WeakCallback(
      const WeakCallbackData<Promise, CallbackInfo>&);
  inline void WeakCallback(Isolate* isolate, Local<Promise> promise);
  inline CallbackInfo(Isolate* isolate, Local<Promise> promise);
  ~CallbackInfo();
  Persistent<Promise> persistent_;
};

CallbackInfo* CallbackInfo::New(Isolate* isolate, Local<Promise> promise) {
  return new CallbackInfo(isolate, promise);
}

CallbackInfo::CallbackInfo(Isolate* isolate, Local<Promise> promise)
    : persistent_(isolate, promise) {
  persistent_.SetWeak(this, WeakCallback);
  persistent_.MarkIndependent();
}

CallbackInfo::~CallbackInfo() {
  persistent_.Reset();
}

void CallbackInfo::WeakCallback(
    const WeakCallbackData<Promise, CallbackInfo>& data) {
/* debug:start */
fprintf(stderr, "CallbackInfo::WeakCallback\n");
/* debug:stop */
  data.GetParameter()->WeakCallback(data.GetIsolate(), data.GetValue());
}

void CallbackInfo::WeakCallback(Isolate* isolate, Local<Promise> promise) {
  delete this;
}

void MakeWeak(const FunctionCallbackInfo<Value>& args) {
  assert(args[0]->IsPromise());
  CallbackInfo::New(args.GetIsolate(), args[0].As<Promise>());
}

void Init(Handle<Object> target) {
  NODE_SET_METHOD(target, "makeWeak", MakeWeak);
}

NODE_MODULE(addon, Init)

and the JS:

'use strict';
const makeWeak = require('./build/Release/addon').makeWeak;

(function() {
  for (var i = 0; i < 1e3; i++)
    makeWeak(Promise.reject(1));
}());

gc();

I've run this against master and also the v8 4.9 branch. if you change Promise.reject() to Promise.resolve() you'll see messages on the console. Otherwise there are none. So for some reason rejected Promises don't trigger the weak callback.

/cc @ofrobots @bnoordhuis

[vkurchatkin]

@trevnorris

So for some reason rejected Promises don't trigger the weak callback.

This makes sense, we have strong references to rejected promises as a part of unhandledRejection implementation.

[Fishrock123]

@vkurchatkin we get rid of those after we fire/default on unhandledRejection -- even your example shows that.

[vkurchatkin]

@Fishrock123 yep, but in @trevnorris's example gc() is called too early

[Fishrock123]

Oh hmmm, even your example uses gc()..

[Fishrock123]

I've updated this. Contains some debug code, I haven't quite gotten the NativeWeakMap to work yet.

"Natural GC" Test case:

var p = new Promise(function(res, rej) {
  console.log('this is a promise!')
  consol.log('oops')
});

// do some stuff, spend some time, idk
var i = 200
var timeout = setInterval(() => {
  var j = 50;
  var string;
  while (j--) {
    var buf = new Buffer(50000);
    string = buf.toString('utf8')
  }
  var what = { i }
  if (i === 0) {
    clearTimeout(timeout)
  }
  i--
}, 20)

[Fishrock123]

Currently testing the fast-exist case:

new Promise(function(res, rej) {
  console.log('this is a promise!')
  consol.log('oops')
});

Current output is:

this is a promise!
set it..

... I'm not sure why printf("had it.."); isn't getting hit.

[Fishrock123]

Looks like it's working. CI: https://ci.nodejs.org/job/node-test-pull-request/2376/

[bnoordhuis]

Why the HandleScope?

[Fishrock123]

I'm pretty sure I needed it to make new Locals?

[bnoordhuis]

JS -> C++ callbacks have an implicit HandleScope, you don't need to create one yourself.

[Fishrock123]

Making a new PR.