chakrashim: Fix Promise::Resolver

[kfarnung]

The promisify function calls into native code to create a new
Promise::Resolver object and returns it back to script. In script it's
expected to be a promise object, but in our case it wasn't.

The solution is to hide the resolver data in an external object and
attach it to the promise object.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

chakrashim

[kfarnung]

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/197/
Linux rerun: https://ci.nodejs.org/job/chakracore-test-linux/284/

[kfarnung]

I still need to look at a strategy for testing this behavior, it's somewhat odd since the native code returns a Promise::Resolver, but the script gets back a promise. That's why we didn't catch this earlier.

Native code:

node-chakracore/src/node_util.cc

Lines 149 to 154 in b7ed19b

	void CreatePromise(const FunctionCallbackInfo<Value>& args) {
	Local<Context> context = args.GetIsolate()->GetCurrentContext();
	auto maybe_resolver = Promise::Resolver::New(context);
	if (!maybe_resolver.IsEmpty())
	args.GetReturnValue().Set(maybe_resolver.ToLocalChecked());
	}

Script usage:

node-chakracore/lib/internal/util.js

Lines 290 to 309 in 176336f

	function fn(...args) {
	const promise = createPromise();
	try {
	original.call(this, ...args, (err, ...values) => {
	if (err) {
	promiseReject(promise, err);
	} else if (argumentNames !== undefined && values.length > 1) {
	const obj = {};
	for (var i = 0; i < argumentNames.length; i++)
	obj[argumentNames[i]] = values[i];
	promiseResolve(promise, obj);
	} else {
	promiseResolve(promise, values[0]);
	}
	});
	} catch (err) {
	promiseReject(promise, err);
	}
	return promise;
	}

[digitalinfinity]

Are chakrashim's news throwing? If not, need a check here that data is not null

[kfarnung]

Good question, I believe they are, but I'm not sure. Looking at the other instances of new I don't see any null checks, so if it's no-throw then we'll have to clean them all up.

[digitalinfinity]

This is very similar to what @MSLaguana fixed upstream with nodejs/node#18765. Maybe a better fix is to just fix CreatePromise in node_util.cc to do the following:

 void CreatePromise(const FunctionCallbackInfo<Value>& args) { 
   Local<Context> context = args.GetIsolate()->GetCurrentContext(); 

   auto maybe_resolver = Promise::Resolver::New(context); 
   if (!maybe_resolver.IsEmpty()) 
     args.GetReturnValue().Set(maybe_resolver->GetPromise().ToLocalChecked()); 
 } 

(Or something like that 😄)

Where are we hitting this btw?

[kfarnung]

@digitalinfinity Unfortunately that won't work, I did consider Jimmy's upstream fix, but the object that's returned need to be both:

• A valid promise object
• Able to carry the resolve/reject functions along

Jimmy's fix requires that someone keep track of the Promise::Resolver, but in this case it's not stored anywhere outside of the calling script. The change he made upstream was the right fix there, nobody was ever attempting to take a promise and get a resolver from it.

I've been making a point to use node-chakracore for basically everything and I hit some really weird behavior in https://github.com/nodejs/node-core-utils which uses util.promisify to wrap the fs calls:

https://github.com/nodejs/node-core-utils/blob/a15715ad1434ed12287adf15d00ed35123a654a8/lib/auth.js#L9-L11

I think there may also be another bug because when createPromise was returning the wrapper object, the code seemed to get into an infinite loop.

[digitalinfinity]

Presumably because of code like the following? There seems to be widespread assumptions that a Promise is a Resolver, which is not our model I guess.

node-chakracore/src/node_util.cc

Line 162 in b7ed19b

Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>(); // sic

[kfarnung]

Yeah, that's the problematic pattern. The good news is that with this change a Resolver is a Promise (but not the other way around). Looking at the V8 API surface this appears to be the pattern as well. There's no way to create Promise object directly, you create a resolver and then get the promise from it.

The thing we didn't have yet was being able to cast that Promise back to a resolver (which is what this change enables).

[MSLaguana]

Ugh, it pains me that this is necessary, but it looks good to me.

[MSLaguana]

Also by my understanding of this change, a v8::Promise (constructed via the shim) is equivalent to a v8::Promise::Resolver. I was just about to say that node code probably wouldn't get a value and go "Oh it's a promise, let me get the resolver" but now that I think about it, there is that whole IsPromise property that is available...

[kfarnung]

@MSLaguana I don't believe there was a way to create a promise via the shim prior to your change. The V8 API requires you to create a Promise::Resolver and then get the Promise from it (there is no New method and the constructor is private). Even though their own documentation doesn't indicate it, they seem to allow casting back and forth between them. Further, if a resolver is returns to script it seems to implicitly cast it to a Promise.

If you create the promise some other way (via N-API or script), then it won't be equivalent, but on the typical path where you are only using the V8 surface, that's correct.

[kfarnung]

There appears to be a problem with the Windows CI /cc @joaocgreis

I've run on the VSTS CI and everything passes, so I'm going to land this change.

[joaocgreis]

The CI issue was an infra problem, one of the machines connected to jenkins to run jobs ran out of memory. This has already been fixed, and I did not see the problem again with chakracore-test-windows.