Skip to content

Commit

Permalink
lib,src: replace all C++ promises with JS promises
Browse files Browse the repository at this point in the history
C++ promises can not be properly optimized by V8. They also behave
a tiny bit different than "regular" promises.

PR-URL: #20830
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Backport-PR-URL: #21879
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
  • Loading branch information
BridgeAR authored and targos committed Jul 24, 2018
1 parent b98bf82 commit 9817e40
Show file tree
Hide file tree
Showing 7 changed files with 27 additions and 123 deletions.
23 changes: 10 additions & 13 deletions lib/child_process.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ const {
deprecate, convertToValidSignal, getSystemErrorName
} = require('internal/util');
const { isUint8Array } = require('internal/util/types');
const { createPromise,
promiseResolve, promiseReject } = process.binding('util');
const debug = util.debuglog('child_process');
const { Buffer } = require('buffer');
const { Pipe, constants: PipeConstants } = process.binding('pipe_wrap');
Expand Down Expand Up @@ -152,18 +150,17 @@ exports.exec = function exec(command /* , options, callback */) {

const customPromiseExecFunction = (orig) => {
return (...args) => {
const promise = createPromise();

orig(...args, (err, stdout, stderr) => {
if (err !== null) {
err.stdout = stdout;
err.stderr = stderr;
promiseReject(promise, err);
} else {
promiseResolve(promise, { stdout, stderr });
}
return new Promise((resolve, reject) => {
orig(...args, (err, stdout, stderr) => {
if (err !== null) {
err.stdout = stdout;
err.stderr = stderr;
reject(err);
} else {
resolve({ stdout, stderr });
}
});
});
return promise;
};
};

Expand Down
5 changes: 1 addition & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,10 +207,7 @@ function exists(path, callback) {

Object.defineProperty(exists, internalUtil.promisify.custom, {
value: (path) => {
const { createPromise, promiseResolve } = process.binding('util');
const promise = createPromise();
fs.exists(path, (exists) => promiseResolve(promise, exists));
return promise;
return new Promise((resolve) => fs.exists(path, resolve));
}
});

Expand Down
9 changes: 3 additions & 6 deletions lib/internal/http2/core.js
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,6 @@ const { isArrayBufferView } = require('internal/util/types');
const { FileHandle } = process.binding('fs');
const binding = process.binding('http2');
const { ShutdownWrap } = process.binding('stream_wrap');
const { createPromise, promiseResolve } = process.binding('util');
const { UV_EOF } = process.binding('uv');

const { StreamPipe } = internalBinding('stream_pipe');
Expand Down Expand Up @@ -2747,11 +2746,9 @@ function connect(authority, options, listener) {
// Support util.promisify
Object.defineProperty(connect, promisify.custom, {
value: (authority, options) => {
const promise = createPromise();
const server = connect(authority,
options,
() => promiseResolve(promise, server));
return promise;
return new Promise((resolve) => {
const server = connect(authority, options, () => resolve(server));
});
}
});

Expand Down
20 changes: 7 additions & 13 deletions lib/internal/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ const {
const { signals } = process.binding('constants').os;

const {
createPromise,
getHiddenValue,
promiseResolve,
promiseReject,
setHiddenValue,
arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex,
decorated_private_symbol: kDecoratedPrivateSymbolIndex
Expand Down Expand Up @@ -276,24 +273,21 @@ function promisify(original) {
const argumentNames = original[kCustomPromisifyArgsSymbol];

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

Object.setPrototypeOf(fn, Object.getPrototypeOf(original));
Expand Down
17 changes: 6 additions & 11 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ const {
validateTimerDuration
} = require('internal/timers');
const internalUtil = require('internal/util');
const { createPromise, promiseResolve } = process.binding('util');
const assert = require('assert');
const util = require('util');
const { ERR_INVALID_CALLBACK } = require('internal/errors').codes;
Expand Down Expand Up @@ -407,11 +406,9 @@ function setTimeout(callback, after, arg1, arg2, arg3) {
}

setTimeout[internalUtil.promisify.custom] = function(after, value) {
const promise = createPromise();
const timeout = new Timeout(promise, after, [value], false, false);
active(timeout);

return promise;
return new Promise((resolve) => {
active(new Timeout(resolve, after, [value], false, false));
});
};

exports.setTimeout = setTimeout;
Expand All @@ -420,7 +417,7 @@ exports.setTimeout = setTimeout;
function ontimeout(timer, start) {
const args = timer._timerArgs;
if (typeof timer._onTimeout !== 'function')
return promiseResolve(timer._onTimeout, args[0]);
return Promise.resolve(timer._onTimeout, args[0]);
if (start === undefined && timer._repeat)
start = TimerWrap.now();
if (!args)
Expand Down Expand Up @@ -691,7 +688,7 @@ function tryOnImmediate(immediate, oldTail, count, refCount) {
function runCallback(timer) {
const argv = timer._argv;
if (typeof timer._onImmediate !== 'function')
return promiseResolve(timer._onImmediate, argv[0]);
return Promise.resolve(timer._onImmediate, argv[0]);
if (!argv)
return timer._onImmediate();
Reflect.apply(timer._onImmediate, timer, argv);
Expand Down Expand Up @@ -766,9 +763,7 @@ function setImmediate(callback, arg1, arg2, arg3) {
}

setImmediate[internalUtil.promisify.custom] = function(value) {
const promise = createPromise();
new Immediate(promise, [value]);
return promise;
return new Promise((resolve) => new Immediate(resolve, [value]));
};

exports.setImmediate = setImmediate;
Expand Down
35 changes: 0 additions & 35 deletions src/node_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ using v8::Context;
using v8::FunctionCallbackInfo;
using v8::Integer;
using v8::Local;
using v8::Maybe;
using v8::Object;
using v8::Private;
using v8::Promise;
Expand Down Expand Up @@ -127,36 +126,6 @@ void WatchdogHasPendingSigint(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(ret);
}


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());
}


void PromiseResolve(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = args.GetIsolate()->GetCurrentContext();
Local<Value> promise = args[0];
CHECK(promise->IsPromise());
if (promise.As<Promise>()->State() != Promise::kPending) return;
Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>(); // sic
Maybe<bool> ret = resolver->Resolve(context, args[1]);
args.GetReturnValue().Set(ret.FromMaybe(false));
}


void PromiseReject(const FunctionCallbackInfo<Value>& args) {
Local<Context> context = args.GetIsolate()->GetCurrentContext();
Local<Value> promise = args[0];
CHECK(promise->IsPromise());
if (promise.As<Promise>()->State() != Promise::kPending) return;
Local<Promise::Resolver> resolver = promise.As<Promise::Resolver>(); // sic
Maybe<bool> ret = resolver->Reject(context, args[1]);
args.GetReturnValue().Set(ret.FromMaybe(false));
}

void SafeGetenv(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsString());
Utf8Value strenvtag(args.GetIsolate(), args[0]);
Expand Down Expand Up @@ -211,10 +180,6 @@ void Initialize(Local<Object> target,
env->SetMethodNoSideEffect(target, "watchdogHasPendingSigint",
WatchdogHasPendingSigint);

env->SetMethodNoSideEffect(target, "createPromise", CreatePromise);
env->SetMethod(target, "promiseResolve", PromiseResolve);
env->SetMethod(target, "promiseReject", PromiseReject);

env->SetMethod(target, "safeGetenv", SafeGetenv);
}

Expand Down
41 changes: 0 additions & 41 deletions test/parallel/test-promise-internal-creation.js

This file was deleted.

0 comments on commit 9817e40

Please sign in to comment.