Skip to content

Commit

Permalink
src: add receiver to fast api callback methods
Browse files Browse the repository at this point in the history
When creating an fast api the callback might use the receiver. In that
case if the internal binding is destructured the method won't have
access to the reciver and it will throw. Passing the receiver as second
argument ensures the receiver is available.

PR-URL: nodejs#54408
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
  • Loading branch information
Ceres6 authored and louwers committed Nov 2, 2024
1 parent d3bc007 commit 113f97b
Show file tree
Hide file tree
Showing 18 changed files with 123 additions and 53 deletions.
2 changes: 1 addition & 1 deletion doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ The initializer module also needs to be allowed. Consider the following example:
```console
$ node --experimental-permission t.js
node:internal/modules/cjs/loader:162
const result = internalModuleStat(filename);
const result = internalModuleStat(receiver, filename);
^

Error: Access to this API has been restricted
Expand Down
2 changes: 1 addition & 1 deletion doc/api/permissions.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ will be restricted.
```console
$ node --experimental-permission index.js
node:internal/modules/cjs/loader:171
const result = internalModuleStat(filename);
const result = internalModuleStat(receiver, filename);
^

Error: Access to this API has been restricted
Expand Down
19 changes: 18 additions & 1 deletion doc/contributing/adding-v8-fast-api.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ for example, they may not trigger garbage collection.
[`node_external_reference.h`](../../src/node_external_reference.h) file.
Although, it would not start failing or crashing until the function ends up
in a snapshot (either the built-in or a user-land one). Please refer to the
[binding functions documentation](../../src#binding-functions) for more
[binding functions documentation](../../src/README.md#binding-functions) for more
information.
* To test fast APIs, make sure to run the tests in a loop with a decent
iterations count to trigger relevant optimizations that prefer the fast API
Expand All @@ -38,6 +38,23 @@ for example, they may not trigger garbage collection.
* The fast callback must be idempotent up to the point where error and fallback
conditions are checked, because otherwise executing the slow callback might
produce visible side effects twice.
* If the receiver is used in the callback, it must be passed as a second argument,
leaving the first one unused, to prevent the JS land from accidentally omitting the receiver when
invoking the fast API method.

```cpp
// Instead of invoking the method as `receiver.internalModuleStat(input)`, the JS land should
// invoke it as `internalModuleStat(binding, input)` to make sure the binding is available to
// the native land.
static int32_t FastInternalModuleStat(
Local<Object> unused,
Local<Object> recv,
const FastOneByteString& input,
FastApiCallbackOptions& options) {
Environment* env = Environment::GetCurrent(recv->GetCreationContextChecked());
// More code
}
```
## Fallback to slow path
Expand Down
2 changes: 1 addition & 1 deletion lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,7 @@ function readdirSyncRecursive(basePath, options) {
for (let i = 0; i < readdirResult.length; i++) {
const resultPath = pathModule.join(path, readdirResult[i]);
const relativeResultPath = pathModule.relative(basePath, resultPath);
const stat = binding.internalModuleStat(resultPath);
const stat = binding.internalModuleStat(binding, resultPath);
ArrayPrototypePush(readdirResults, relativeResultPath);
// 1 indicates directory
if (stat === 1) {
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ async function readdirRecursive(originalPath, options) {
const { 0: path, 1: readdir } = ArrayPrototypePop(queue);
for (const ent of readdir) {
const direntPath = pathModule.join(path, ent);
const stat = binding.internalModuleStat(direntPath);
const stat = binding.internalModuleStat(binding, direntPath);
ArrayPrototypePush(
result,
pathModule.relative(originalPath, direntPath),
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,9 @@ function stat(filename) {
const result = statCache.get(filename);
if (result !== undefined) { return result; }
}
const result = internalFsBinding.internalModuleStat(filename);
const result = internalFsBinding.internalModuleStat(internalFsBinding, filename);
if (statCache !== null && result >= 0) {
// Only set cache when `internalModuleStat(filename)` succeeds.
// Only set cache when `internalModuleStat(internalFsBinding, filename)` succeeds.
statCache.set(filename, result);
}
return result;
Expand Down
7 changes: 5 additions & 2 deletions lib/internal/modules/esm/resolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,10 @@ function finalizeResolution(resolved, base, preserveSymlinks) {
throw err;
}

const stats = internalFsBinding.internalModuleStat(StringPrototypeEndsWith(path, '/') ?
StringPrototypeSlice(path, -1) : path);
const stats = internalFsBinding.internalModuleStat(
internalFsBinding,
StringPrototypeEndsWith(internalFsBinding, path, '/') ? StringPrototypeSlice(path, -1) : path,
);

// Check for stats.isDirectory()
if (stats === 1) {
Expand Down Expand Up @@ -802,6 +804,7 @@ function packageResolve(specifier, base, conditions) {
let lastPath;
do {
const stat = internalFsBinding.internalModuleStat(
internalFsBinding,
StringPrototypeSlice(packageJSONPath, 0, packageJSONPath.length - 13),
);
// Check for !stat.isDirectory()
Expand Down
31 changes: 19 additions & 12 deletions src/histogram.cc
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ void HistogramBase::RecordDelta(const FunctionCallbackInfo<Value>& args) {
(*histogram)->RecordDelta();
}

void HistogramBase::FastRecordDelta(Local<Value> receiver) {
void HistogramBase::FastRecordDelta(Local<Value> unused,
Local<Value> receiver) {
HistogramBase* histogram;
ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver);
(*histogram)->RecordDelta();
Expand All @@ -190,7 +191,8 @@ void HistogramBase::Record(const FunctionCallbackInfo<Value>& args) {
(*histogram)->Record(value);
}

void HistogramBase::FastRecord(Local<Value> receiver,
void HistogramBase::FastRecord(Local<Value> unused,
Local<Value> receiver,
const int64_t value,
FastApiCallbackOptions& options) {
if (value < 1) {
Expand Down Expand Up @@ -438,7 +440,9 @@ void IntervalHistogram::Start(const FunctionCallbackInfo<Value>& args) {
histogram->OnStart(args[0]->IsTrue() ? StartFlags::RESET : StartFlags::NONE);
}

void IntervalHistogram::FastStart(Local<Value> receiver, bool reset) {
void IntervalHistogram::FastStart(Local<Value> unused,
Local<Value> receiver,
bool reset) {
IntervalHistogram* histogram;
ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver);
histogram->OnStart(reset ? StartFlags::RESET : StartFlags::NONE);
Expand All @@ -450,7 +454,7 @@ void IntervalHistogram::Stop(const FunctionCallbackInfo<Value>& args) {
histogram->OnStop();
}

void IntervalHistogram::FastStop(Local<Value> receiver) {
void IntervalHistogram::FastStop(Local<Value> unused, Local<Value> receiver) {
IntervalHistogram* histogram;
ASSIGN_OR_RETURN_UNWRAP(&histogram, receiver);
histogram->OnStop();
Expand Down Expand Up @@ -566,42 +570,45 @@ void HistogramImpl::DoReset(const FunctionCallbackInfo<Value>& args) {
(*histogram)->Reset();
}

void HistogramImpl::FastReset(Local<Value> receiver) {
void HistogramImpl::FastReset(Local<Value> unused, Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
(*histogram)->Reset();
}

double HistogramImpl::FastGetCount(Local<Value> receiver) {
double HistogramImpl::FastGetCount(Local<Value> unused, Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return static_cast<double>((*histogram)->Count());
}

double HistogramImpl::FastGetMin(Local<Value> receiver) {
double HistogramImpl::FastGetMin(Local<Value> unused, Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return static_cast<double>((*histogram)->Min());
}

double HistogramImpl::FastGetMax(Local<Value> receiver) {
double HistogramImpl::FastGetMax(Local<Value> unused, Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return static_cast<double>((*histogram)->Max());
}

double HistogramImpl::FastGetMean(Local<Value> receiver) {
double HistogramImpl::FastGetMean(Local<Value> unused, Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return (*histogram)->Mean();
}

double HistogramImpl::FastGetExceeds(Local<Value> receiver) {
double HistogramImpl::FastGetExceeds(Local<Value> unused,
Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return static_cast<double>((*histogram)->Exceeds());
}

double HistogramImpl::FastGetStddev(Local<Value> receiver) {
double HistogramImpl::FastGetStddev(Local<Value> unused,
Local<Value> receiver) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return (*histogram)->Stddev();
}

double HistogramImpl::FastGetPercentile(Local<Value> receiver,
double HistogramImpl::FastGetPercentile(Local<Value> unused,
Local<Value> receiver,
const double percentile) {
HistogramImpl* histogram = HistogramImpl::FromJSObject(receiver);
return static_cast<double>((*histogram)->Percentile(percentile));
Expand Down
35 changes: 24 additions & 11 deletions src/histogram.h
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,22 @@ class HistogramImpl {
static void GetPercentilesBigInt(
const v8::FunctionCallbackInfo<v8::Value>& args);

static void FastReset(v8::Local<v8::Value> receiver);
static double FastGetCount(v8::Local<v8::Value> receiver);
static double FastGetMin(v8::Local<v8::Value> receiver);
static double FastGetMax(v8::Local<v8::Value> receiver);
static double FastGetMean(v8::Local<v8::Value> receiver);
static double FastGetExceeds(v8::Local<v8::Value> receiver);
static double FastGetStddev(v8::Local<v8::Value> receiver);
static double FastGetPercentile(v8::Local<v8::Value> receiver,
static void FastReset(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetCount(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetMin(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetMax(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetMean(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetExceeds(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetStddev(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
static double FastGetPercentile(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver,
const double percentile);

static void AddMethods(v8::Isolate* isolate,
Expand Down Expand Up @@ -158,10 +166,12 @@ class HistogramBase final : public BaseObject, public HistogramImpl {
static void Add(const v8::FunctionCallbackInfo<v8::Value>& args);

static void FastRecord(
v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver,
const int64_t value,
v8::FastApiCallbackOptions& options); // NOLINT(runtime/references)
static void FastRecordDelta(v8::Local<v8::Value> receiver);
static void FastRecordDelta(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);

HistogramBase(
Environment* env,
Expand Down Expand Up @@ -233,8 +243,11 @@ class IntervalHistogram final : public HandleWrap, public HistogramImpl {
static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Stop(const v8::FunctionCallbackInfo<v8::Value>& args);

static void FastStart(v8::Local<v8::Value> receiver, bool reset);
static void FastStop(v8::Local<v8::Value> receiver);
static void FastStart(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver,
bool reset);
static void FastStop(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);

BaseObject::TransferMode GetTransferMode() const override {
return TransferMode::kCloneable;
Expand Down
23 changes: 17 additions & 6 deletions src/node_external_reference.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,25 @@ namespace node {

using CFunctionCallbackWithOneByteString =
uint32_t (*)(v8::Local<v8::Value>, const v8::FastOneByteString&);
using CFunctionCallback = void (*)(v8::Local<v8::Value> receiver);
using CFunctionCallback = void (*)(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver);
using CFunctionCallbackReturnDouble =
double (*)(v8::Local<v8::Object> receiver);
double (*)(v8::Local<v8::Object> unused, v8::Local<v8::Object> receiver);
using CFunctionCallbackReturnInt32 =
int32_t (*)(v8::Local<v8::Object> receiver,
int32_t (*)(v8::Local<v8::Object> unused,
v8::Local<v8::Object> receiver,
const v8::FastOneByteString& input,
// NOLINTNEXTLINE(runtime/references) This is V8 api.
v8::FastApiCallbackOptions& options);
using CFunctionCallbackValueReturnDouble =
double (*)(v8::Local<v8::Value> receiver);
using CFunctionCallbackWithInt64 = void (*)(v8::Local<v8::Object> receiver,
using CFunctionCallbackValueReturnDoubleUnusedReceiver =
double (*)(v8::Local<v8::Value> unused, v8::Local<v8::Value> receiver);
using CFunctionCallbackWithInt64 = void (*)(v8::Local<v8::Object> unused,
v8::Local<v8::Object> receiver,
int64_t);
using CFunctionCallbackWithBool = void (*)(v8::Local<v8::Object> receiver,
using CFunctionCallbackWithBool = void (*)(v8::Local<v8::Object> unused,
v8::Local<v8::Object> receiver,
bool);
using CFunctionCallbackWithString =
bool (*)(v8::Local<v8::Value>, const v8::FastOneByteString& input);
Expand All @@ -50,11 +56,15 @@ using CFunctionCallbackWithUint8ArrayUint32Int64Bool =
using CFunctionWithUint32 = uint32_t (*)(v8::Local<v8::Value>,
const uint32_t input);
using CFunctionWithDoubleReturnDouble = double (*)(v8::Local<v8::Value>,
v8::Local<v8::Value>,
const double);
using CFunctionWithInt64Fallback = void (*)(v8::Local<v8::Value>,
v8::Local<v8::Value>,
const int64_t,
v8::FastApiCallbackOptions&);
using CFunctionWithBool = void (*)(v8::Local<v8::Value>, bool);
using CFunctionWithBool = void (*)(v8::Local<v8::Value>,
v8::Local<v8::Value>,
bool);

using CFunctionWriteString =
uint32_t (*)(v8::Local<v8::Value> receiver,
Expand Down Expand Up @@ -83,6 +93,7 @@ class ExternalReferenceRegistry {
V(CFunctionCallbackReturnDouble) \
V(CFunctionCallbackReturnInt32) \
V(CFunctionCallbackValueReturnDouble) \
V(CFunctionCallbackValueReturnDoubleUnusedReceiver) \
V(CFunctionCallbackWithInt64) \
V(CFunctionCallbackWithBool) \
V(CFunctionCallbackWithString) \
Expand Down
6 changes: 4 additions & 2 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1040,8 +1040,9 @@ static void ExistsSync(const FunctionCallbackInfo<Value>& args) {
static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsString());
BufferValue path(env->isolate(), args[0]);
CHECK_GE(args.Length(), 2);
CHECK(args[1]->IsString());
BufferValue path(env->isolate(), args[1]);
CHECK_NOT_NULL(*path);
ToNamespacedPath(env, &path);
THROW_IF_INSUFFICIENT_PERMISSIONS(
Expand All @@ -1059,6 +1060,7 @@ static void InternalModuleStat(const FunctionCallbackInfo<Value>& args) {
}

static int32_t FastInternalModuleStat(
Local<Object> unused,
Local<Object> recv,
const FastOneByteString& input,
// NOLINTNEXTLINE(runtime/references) This is V8 api.
Expand Down
6 changes: 4 additions & 2 deletions src/node_process.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,15 +72,17 @@ class BindingData : public SnapshotableObject {
static BindingData* FromV8Value(v8::Local<v8::Value> receiver);
static void NumberImpl(BindingData* receiver);

static void FastNumber(v8::Local<v8::Value> receiver) {
static void FastNumber(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver) {
NumberImpl(FromV8Value(receiver));
}

static void SlowNumber(const v8::FunctionCallbackInfo<v8::Value>& args);

static void BigIntImpl(BindingData* receiver);

static void FastBigInt(v8::Local<v8::Value> receiver) {
static void FastBigInt(v8::Local<v8::Value> unused,
v8::Local<v8::Value> receiver) {
BigIntImpl(FromV8Value(receiver));
}

Expand Down
1 change: 1 addition & 0 deletions src/node_wasi.cc
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ inline void EinvalError() {}

template <typename FT, FT F, typename R, typename... Args>
R WASI::WasiFunction<FT, F, R, Args...>::FastCallback(
Local<Object> unused,
Local<Object> receiver,
Args... args,
// NOLINTNEXTLINE(runtime/references) This is V8 api.
Expand Down
3 changes: 2 additions & 1 deletion src/node_wasi.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,8 @@ class WASI : public BaseObject,
v8::Local<v8::FunctionTemplate>);

private:
static R FastCallback(v8::Local<v8::Object> receiver,
static R FastCallback(v8::Local<v8::Object> unused,
v8::Local<v8::Object> receiver,
Args...,
v8::FastApiCallbackOptions&);

Expand Down
Loading

0 comments on commit 113f97b

Please sign in to comment.