Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib,src: replace all C++ promises with JS promises #20830

Closed
wants to merge 1 commit into from
Closed

lib,src: replace all C++ promises with JS promises #20830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions lib/child_process.js
View file
Open in desktop
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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was written that way to avoid allocation of a closure and be speed-competitive with bluebird.

I’d like to make sure that the speed remains as fast

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should measure the performance here.

I was suggesting to start with JS promises instead, since we spend some time recently to even inline the promise constructor into TurboFan optimized code. If that turns out to be too slow, then the other alternative is to use the createPromise and friends from v8-extras, which is also known to TurboFan, and significantly faster than going through C++.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for createPromise from v8-extras.

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,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
View file
Open in desktop
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,
Copy link
Member

@TimothyGu TimothyGu Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it seems we might be able to remove the custom promisifier entirely: what the getter is doing right now should be the exact things done by util.promisify.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test fails if that part is removed.

options,
() => promiseResolve(promise, server));
return promise;
return new Promise((resolve) => {
const server = connect(authority, options, () => resolve(server));
Copy link
Member

@benjamingr benjamingr May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes existing behavior - if an invalid URL is passed as an authority here then the function throws. The old behavior was to throw synchronously but the new behavior throws asynchronously.

In my opinion the new behavior is a behavior is better - but it's a change in error messages though. I'm not sure how it can be resolved in the promise constructor or done without reverting the change.

What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could rewrite it to behave the same as before when doing:

let callback;
const server = connect(authority, options, () => callback(server));
return new Promise((resolve) => {
  callback = resolve;
});

However, I do think it is best to handle it async.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think it's best to handle it async too but I'm not sure I get to make that call. Pinging previous approvers for more OKs - @addaleax @bmeurer @TimothyGu @devsnek - please 👍 this comment to agree to landing this (arguably bugfix) behaviour change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a breaking change – otherwise no strong opinions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax I do not see how someone would really rely on this.

Most people will probably not wrap a promise call in a try catch. So it'll end up as uncaught exception and that will likely end the process. That is not really something that anyone would want.

What scenario do you see that would break with this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in experimental http2 code. There no reason to make it semver-major on the basis of this one change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, http2 is still experimental. Fine with me then. :)

(I had originally added the dont-land-on-v10.x label because of perf concerns anyway – feel free to remove it if you like.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not realize http2 is still experimental either - I'm fine with semver-patch :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax

How important is this? Do you want to have this patch in 10.x for another reason besides the behavioral improvement here?

Backporting is easier as well and users get the better behavior earlier :-) (even though that only makes sense for 10.).

I do not have a strong opinion on it though. If you want, I can change the timers and the child_process to behave exactly as before but I personally still believe this is a bug fix as it was never meant to behave like that (and it only did that in the first place because of the special implementation before).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BridgeAR IIUC @addaleax is fine landing this as semver patch #20830 (comment) and later added:

(I had originally added the dont-land-on-v10.x label because of perf concerns anyway – feel free to remove it if you like.)

Which means that this is fine to land as semver patch and on v10.x

});
}
});

Expand Down
20 changes: 7 additions & 13 deletions lib/internal/util.js
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ const {
validateTimerDuration
} = require('internal/timers');
const internalUtil = require('internal/util');
const { createPromise, promiseResolve } = process.binding('util');
const util = require('util');
const { ERR_INVALID_CALLBACK } = require('internal/errors').codes;
const debug = util.debuglog('timer');
Expand Down Expand Up @@ -439,11 +438,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);
active(timeout);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think active can throw but if it can this is also a behavior change. In any case maybe this can just be return new Promise(resolve => setTimeout(resolve, after, value))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski PTAL.

});
};

exports.setTimeout = setTimeout;
Expand All @@ -452,7 +449,7 @@ exports.setTimeout = setTimeout;
function ontimeout(timer) {
const args = timer._timerArgs;
if (typeof timer._onTimeout !== 'function')
return promiseResolve(timer._onTimeout, args[0]);
return Promise.resolve(timer._onTimeout, args[0]);
if (!args)
timer._onTimeout();
else
Expand Down Expand Up @@ -659,7 +656,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 @@ -738,9 +735,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
View file
Open in desktop
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 @@ -140,36 +139,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 @@ -224,10 +193,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
View file
Open in desktop

This file was deleted.