Skip to content

Commit

Permalink
src: pass along errors from fs object creations
Browse files Browse the repository at this point in the history 
PR-URL: #25822
Reviewed-By: Gireesh Punathil <[email protected]>
  • Loading branch information
@addaleax
addaleax committed Feb 8, 2019
1 parent 0672c24 commit d6f3b87
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 37 deletions.
66 changes: 47 additions & 19 deletions src/node_file.cc
View file
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
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,
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

0 comments on commit d6f3b87

Please sign in to comment.