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

src: pass along errors from object instantiations (part II) #25822

Closed
wants to merge 4 commits into from
Closed

src: pass along errors from object instantiations (part II) #25822

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8ea8fa
src: pass along errors from tls object creation
addaleax Jan 30, 2019
93d39c3
src: pass along errors from http2 object creation
addaleax Jan 30, 2019
0c37ff5
src: pass along errors from fs object creations
addaleax Jan 30, 2019
ce8dcae
src: pass along errors from StreamBase req obj creations
addaleax Jan 30, 2019
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
66 changes: 47 additions & 19 deletions src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,29 @@ typedef void(*uv_fs_callback_t)(uv_fs_t*);
// The FileHandle object wraps a file descriptor and will close it on garbage
// collection if necessary. If that happens, a process warning will be
// emitted (or a fatal exception will occur if the fd cannot be closed.)
FileHandle::FileHandle(Environment* env, int fd, Local<Object> obj)
: AsyncWrap(env,
obj.IsEmpty() ? env->fd_constructor_template()
->NewInstance(env->context()).ToLocalChecked() : obj,
AsyncWrap::PROVIDER_FILEHANDLE),
FileHandle::FileHandle(Environment* env, Local<Object> obj, int fd)
: AsyncWrap(env, obj, AsyncWrap::PROVIDER_FILEHANDLE),
StreamBase(env),
fd_(fd) {
MakeWeak();
}

FileHandle* FileHandle::New(Environment* env, int fd, Local<Object> obj) {
if (obj.IsEmpty() && !env->fd_constructor_template()
->NewInstance(env->context())
.ToLocal(&obj)) {
return nullptr;
}
v8::PropertyAttribute attr =
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);
object()->DefineOwnProperty(env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "fd"),
Integer::New(env->isolate(), fd),
attr).FromJust();
if (obj->DefineOwnProperty(env->context(),
env->fd_string(),
Integer::New(env->isolate(), fd),
attr)
.IsNothing()) {
return nullptr;
}
return new FileHandle(env, obj, fd);
}

void FileHandle::New(const FunctionCallbackInfo<Value>& args) {
Expand All @@ -132,7 +141,8 @@ void FileHandle::New(const FunctionCallbackInfo<Value>& args) {
CHECK(args[0]->IsInt32());

FileHandle* handle =
new FileHandle(env, args[0].As<Int32>()->Value(), args.This());
FileHandle::New(env, args[0].As<Int32>()->Value(), args.This());
if (handle == nullptr) return;
if (args[1]->IsNumber())
handle->read_offset_ = args[1]->IntegerValue(env->context()).FromJust();
if (args[2]->IsNumber())
Expand Down Expand Up @@ -232,7 +242,14 @@ inline MaybeLocal<Promise> FileHandle::ClosePromise() {
CHECK(!reading_);
if (!closed_ && !closing_) {
closing_ = true;
CloseReq* req = new CloseReq(env(), promise, object());
Local<Object> close_req_obj;
if (!env()
->fdclose_constructor_template()
->NewInstance(env()->context())
.ToLocal(&close_req_obj)) {
return MaybeLocal<Promise>();
}
CloseReq* req = new CloseReq(env(), close_req_obj, promise, object());
auto AfterClose = uv_fs_callback_t{[](uv_fs_t* req) {
std::unique_ptr<CloseReq> close(CloseReq::from_req(req));
CHECK_NOT_NULL(close);
Expand Down Expand Up @@ -260,7 +277,9 @@ inline MaybeLocal<Promise> FileHandle::ClosePromise() {
void FileHandle::Close(const FunctionCallbackInfo<Value>& args) {
FileHandle* fd;
ASSIGN_OR_RETURN_UNWRAP(&fd, args.Holder());
args.GetReturnValue().Set(fd->ClosePromise().ToLocalChecked());
Local<Promise> ret;
if (!fd->ClosePromise().ToLocal(&ret)) return;
args.GetReturnValue().Set(ret);
}


Expand Down Expand Up @@ -318,8 +337,13 @@ int FileHandle::ReadStart() {
read_wrap->AsyncReset();
read_wrap->file_handle_ = this;
} else {
Local<Object> wrap_obj = env()->filehandlereadwrap_template()
->NewInstance(env()->context()).ToLocalChecked();
Local<Object> wrap_obj;
if (!env()
->filehandlereadwrap_template()
->NewInstance(env()->context())
.ToLocal(&wrap_obj)) {
return UV_EBUSY;
}
read_wrap.reset(new FileHandleReadWrap(this, wrap_obj));
}
}
Expand Down Expand Up @@ -520,7 +544,8 @@ void AfterOpenFileHandle(uv_fs_t* req) {
FSReqAfterScope after(req_wrap, req);

if (after.Proceed()) {
FileHandle* fd = new FileHandle(req_wrap->env(), req->result);
FileHandle* fd = FileHandle::New(req_wrap->env(), req->result);
if (fd == nullptr) return;
req_wrap->Resolve(fd->object());
}
}
Expand Down Expand Up @@ -724,15 +749,18 @@ inline int SyncCall(Environment* env, Local<Value> ctx, FSReqWrapSync* req_wrap,
return err;
}

// TODO(addaleax): Currently, callers check the return value and assume
// that nullptr indicates a synchronous call, rather than a failure.
// Failure conditions should be disambiguated and handled appropriately.
inline FSReqBase* GetReqWrap(Environment* env, Local<Value> value,
bool use_bigint = false) {
if (value->IsObject()) {
return Unwrap<FSReqBase>(value.As<Object>());
} else if (value->StrictEquals(env->fs_use_promises_symbol())) {
if (use_bigint) {
return new FSReqPromise<uint64_t, BigUint64Array>(env, use_bigint);
return FSReqPromise<uint64_t, BigUint64Array>::New(env, use_bigint);
} else {
return new FSReqPromise<double, Float64Array>(env, use_bigint);
return FSReqPromise<double, Float64Array>::New(env, use_bigint);
}
}
return nullptr;
Expand Down Expand Up @@ -1562,8 +1590,8 @@ static void OpenFileHandle(const FunctionCallbackInfo<Value>& args) {
if (result < 0) {
return; // syscall failed, no need to continue, error info is in ctx
}
HandleScope scope(isolate);
FileHandle* fd = new FileHandle(env, result);
FileHandle* fd = FileHandle::New(env, result);
if (fd == nullptr) return;
args.GetReturnValue().Set(fd->object());
}
}
Expand Down
42 changes: 24 additions & 18 deletions src/node_file.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,17 +237,19 @@ inline Local<Value> FillGlobalStatsArray(Environment* env,
template <typename NativeT = double, typename V8T = v8::Float64Array>
class FSReqPromise : public FSReqBase {
public:
explicit FSReqPromise(Environment* env, bool use_bigint)
: FSReqBase(env,
env->fsreqpromise_constructor_template()
->NewInstance(env->context()).ToLocalChecked(),
AsyncWrap::PROVIDER_FSREQPROMISE,
use_bigint),
stats_field_array_(env->isolate(), kFsStatsFieldsNumber) {
const auto resolver =
Promise::Resolver::New(env->context()).ToLocalChecked();
USE(object()->Set(env->context(), env->promise_string(),
resolver).FromJust());
static FSReqPromise* New(Environment* env, bool use_bigint) {
v8::Local<Object> obj;
if (!env->fsreqpromise_constructor_template()
->NewInstance(env->context())
.ToLocal(&obj)) {
return nullptr;
}
v8::Local<v8::Promise::Resolver> resolver;
if (!v8::Promise::Resolver::New(env->context()).ToLocal(&resolver) ||
obj->Set(env->context(), env->promise_string(), resolver).IsNothing()) {
return nullptr;
}
return new FSReqPromise(env, obj, use_bigint);
}

~FSReqPromise() override {
Expand Down Expand Up @@ -304,6 +306,10 @@ class FSReqPromise : public FSReqBase {
FSReqPromise& operator=(const FSReqPromise&&) = delete;

private:
FSReqPromise(Environment* env, v8::Local<v8::Object> obj, bool use_bigint)
: FSReqBase(env, obj, AsyncWrap::PROVIDER_FSREQPROMISE, use_bigint),
stats_field_array_(env->isolate(), kFsStatsFieldsNumber) {}

bool finished_ = false;
AliasedBuffer<NativeT, V8T> stats_field_array_;
};
Expand Down Expand Up @@ -356,9 +362,9 @@ class FileHandleReadWrap : public ReqWrap<uv_fs_t> {
// the object is garbage collected
class FileHandle : public AsyncWrap, public StreamBase {
public:
FileHandle(Environment* env,
int fd,
v8::Local<v8::Object> obj = v8::Local<v8::Object>());
static FileHandle* New(Environment* env,
int fd,
v8::Local<v8::Object> obj = v8::Local<v8::Object>());
virtual ~FileHandle();

static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down Expand Up @@ -404,19 +410,19 @@ class FileHandle : public AsyncWrap, public StreamBase {
FileHandle& operator=(const FileHandle&&) = delete;

private:
FileHandle(Environment* env, v8::Local<v8::Object> obj, int fd);

// Synchronous close that emits a warning
void Close();
void AfterClose();

class CloseReq : public ReqWrap<uv_fs_t> {
public:
CloseReq(Environment* env,
Copy link
Member

Choose a reason for hiding this comment

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

any reason why CloseReq does not follow the suit - i.e., a private constructor and a static factory?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just that it’s used at only one call site rather than multiple ones. I can add/use a factory constructor 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.

ok, I thought so. I think it is fine; doesn't look like anything beneficial of doing so.

Local<Object> obj,
Local<Promise> promise,
Local<Value> ref)
: ReqWrap(env,
env->fdclose_constructor_template()
->NewInstance(env->context()).ToLocalChecked(),
AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) {
: ReqWrap(env, obj, AsyncWrap::PROVIDER_FILEHANDLECLOSEREQ) {
promise_.Reset(env->isolate(), promise);
ref_.Reset(env->isolate(), ref);
}
Expand Down
Loading