Skip to content

Commit 4b9ba9b

Browse files
jasnelljasnell
committed
fs: encapsulate FSReqWrap more
In further preparation for the Promises enabled fs API, further encapsulate FSReqWrap details. The intent here is to isolate, as much as possible, the functionality of the various fs functions from the details of how the results are notified. The promises implementation will use a `FSReqPromise` alternative to `FSReqWrap` that will use the same API so that both models can be used without changing any of the actual implementation details for the various methods. PR-URL: #18112 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent a7a1ada commit 4b9ba9b

File tree

3 files changed

+67
-115
lines changed

3 files changed

+67
-115
lines changed

‎src/node_file.cc‎

Lines changed: 47 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ using v8::MaybeLocal;
9494
using v8::Number;
9595
using v8::Object;
9696
using v8::String;
97+
using v8::Undefined;
9798
using v8::Value;
9899

99100
#ifndef MIN
@@ -102,32 +103,16 @@ using v8::Value;
102103

103104
#define GET_OFFSET(a) ((a)->IsNumber() ? (a)->IntegerValue() : -1)
104105

105-
FSReqWrap* FSReqWrap::New(Environment* env,
106-
Local<Object> req,
107-
const char* syscall,
108-
const char* data,
109-
enum encoding encoding,
110-
Ownership ownership) {
111-
const bool copy = (data != nullptr && ownership == COPY);
112-
const size_t size = copy ? 1 + strlen(data) : 0;
113-
FSReqWrap* that;
114-
char* const storage = new char[sizeof(*that) + size];
115-
that = new(storage) FSReqWrap(env, req, syscall, data, encoding);
116-
if (copy)
117-
that->data_ = static_cast<char*>(memcpy(that->inline_data(), data, size));
118-
return that;
106+
void FSReqWrap::Reject(Local<Value> reject) {
107+
MakeCallback(env()->oncomplete_string(), 1, &reject);
119108
}
120109

121-
122-
void FSReqWrap::Dispose() {
123-
this->~FSReqWrap();
124-
delete[] reinterpret_cast<char*>(this);
110+
void FSReqWrap::FillStatsArray(const uv_stat_t* stat) {
111+
node::FillStatsArray(env()->fs_stats_field_array(), stat);
125112
}
126113

127-
128-
void FSReqWrap::Reject(Local<Value> reject) {
129-
Local<Value> argv[1] { reject };
130-
MakeCallback(env()->oncomplete_string(), arraysize(argv), argv);
114+
void FSReqWrap::ResolveStat() {
115+
Resolve(Undefined(env()->isolate()));
131116
}
132117

133118
void FSReqWrap::Resolve(Local<Value> value) {
@@ -138,9 +123,26 @@ void FSReqWrap::Resolve(Local<Value> value) {
138123
MakeCallback(env()->oncomplete_string(), arraysize(argv), argv);
139124
}
140125

126+
void FSReqWrap::Init(const char* syscall,
127+
const char* data,
128+
size_t len,
129+
enum encoding encoding) {
130+
syscall_ = syscall;
131+
encoding_ = encoding;
132+
133+
if (data != nullptr) {
134+
CHECK_EQ(data_, nullptr);
135+
buffer_.AllocateSufficientStorage(len + 1);
136+
buffer_.SetLengthAndZeroTerminate(len);
137+
memcpy(*buffer_, data, len);
138+
data_ = *buffer_;
139+
}
140+
}
141+
141142
void NewFSReqWrap(const FunctionCallbackInfo<Value>& args) {
142143
CHECK(args.IsConstructCall());
143-
ClearWrap(args.This());
144+
Environment* env = Environment::GetCurrent(args.GetIsolate());
145+
new FSReqWrap(env, args.This());
144146
}
145147

146148

@@ -150,12 +152,11 @@ FSReqAfterScope::FSReqAfterScope(FSReqWrap* wrap, uv_fs_t* req)
150152
handle_scope_(wrap->env()->isolate()),
151153
context_scope_(wrap->env()->context()) {
152154
CHECK_EQ(wrap_->req(), req);
153-
wrap_->ReleaseEarly(); // Free memory that's no longer used now.
154155
}
155156

156157
FSReqAfterScope::~FSReqAfterScope() {
157158
uv_fs_req_cleanup(wrap_->req());
158-
wrap_->Dispose();
159+
delete wrap_;
159160
}
160161

161162
// TODO(joyeecheung): create a normal context object, and
@@ -195,12 +196,10 @@ void AfterNoArgs(uv_fs_t* req) {
195196
void AfterStat(uv_fs_t* req) {
196197
FSReqWrap* req_wrap = static_cast<FSReqWrap*>(req->data);
197198
FSReqAfterScope after(req_wrap, req);
198-
Environment* env = req_wrap->env();
199199

200200
if (after.Proceed()) {
201-
FillStatsArray(env->fs_stats_field_array(),
202-
static_cast<const uv_stat_t*>(req->ptr));
203-
req_wrap->Resolve(Undefined(req_wrap->env()->isolate()));
201+
req_wrap->FillStatsArray(&req->statbuf);
202+
req_wrap->ResolveStat();
204203
}
205204
}
206205

@@ -222,7 +221,7 @@ void AfterStringPath(uv_fs_t* req) {
222221
if (after.Proceed()) {
223222
link = StringBytes::Encode(req_wrap->env()->isolate(),
224223
static_cast<const char*>(req->path),
225-
req_wrap->encoding_,
224+
req_wrap->encoding(),
226225
&error);
227226
if (link.IsEmpty())
228227
req_wrap->Reject(error);
@@ -241,7 +240,7 @@ void AfterStringPtr(uv_fs_t* req) {
241240
if (after.Proceed()) {
242241
link = StringBytes::Encode(req_wrap->env()->isolate(),
243242
static_cast<const char*>(req->ptr),
244-
req_wrap->encoding_,
243+
req_wrap->encoding(),
245244
&error);
246245
if (link.IsEmpty())
247246
req_wrap->Reject(error);
@@ -278,7 +277,7 @@ void AfterScanDir(uv_fs_t* req) {
278277
MaybeLocal<Value> filename =
279278
StringBytes::Encode(env->isolate(),
280279
ent.name,
281-
req_wrap->encoding_,
280+
req_wrap->encoding(),
282281
&error);
283282
if (filename.IsEmpty())
284283
return req_wrap->Reject(error);
@@ -317,12 +316,12 @@ class fs_req_wrap {
317316
template <typename Func, typename... Args>
318317
inline FSReqWrap* AsyncDestCall(Environment* env,
319318
const FunctionCallbackInfo<Value>& args,
320-
const char* syscall, const char* dest,
321-
enum encoding enc, FSReqWrap::Ownership ownership,
322-
uv_fs_cb after, Func fn, Args... fn_args) {
319+
const char* syscall, const char* dest, size_t len,
320+
enum encoding enc, uv_fs_cb after, Func fn, Args... fn_args) {
323321
Local<Object> req = args[args.Length() - 1].As<Object>();
324-
FSReqWrap* req_wrap = FSReqWrap::New(env, req,
325-
syscall, dest, enc, ownership);
322+
FSReqWrap* req_wrap = Unwrap<FSReqWrap>(req);
323+
CHECK_NE(req_wrap, nullptr);
324+
req_wrap->Init(syscall, dest, len, enc);
326325
int err = fn(env->event_loop(), req_wrap->req(), fn_args..., after);
327326
req_wrap->Dispatched();
328327
if (err < 0) {
@@ -339,38 +338,16 @@ inline FSReqWrap* AsyncDestCall(Environment* env,
339338
return req_wrap;
340339
}
341340

342-
// Defaults to COPY ownership.
343-
template <typename Func, typename... Args>
344-
inline FSReqWrap* AsyncDestCall(Environment* env,
345-
const FunctionCallbackInfo<Value>& args,
346-
const char* syscall, const char* dest, enum encoding enc,
347-
uv_fs_cb after, Func fn, Args... fn_args) {
348-
return AsyncDestCall(env, args,
349-
syscall, dest, enc, FSReqWrap::COPY,
350-
after, fn, fn_args...);
351-
}
352-
353341
template <typename Func, typename... Args>
354342
inline FSReqWrap* AsyncCall(Environment* env,
355343
const FunctionCallbackInfo<Value>& args,
356-
const char* syscall, enum encoding enc, FSReqWrap::Ownership ownership,
344+
const char* syscall, enum encoding enc,
357345
uv_fs_cb after, Func fn, Args... fn_args) {
358346
return AsyncDestCall(env, args,
359-
syscall, nullptr, enc, ownership,
347+
syscall, nullptr, 0, enc,
360348
after, fn, fn_args...);
361349
}
362350

363-
// Defaults to COPY ownership.
364-
template <typename Func, typename... Args>
365-
inline FSReqWrap* AsyncCall(Environment* env,
366-
const FunctionCallbackInfo<Value>& args,
367-
const char* syscall, enum encoding enc,
368-
uv_fs_cb after, Func fn, Args... fn_args) {
369-
return AsyncCall(env, args,
370-
syscall, enc, FSReqWrap::COPY,
371-
after, fn, fn_args...);
372-
}
373-
374351
// Template counterpart of SYNC_CALL, except that it only puts
375352
// the error number and the syscall in the context instead of
376353
// creating an error in the C++ land.
@@ -623,8 +600,8 @@ static void Symlink(const FunctionCallbackInfo<Value>& args) {
623600

624601
if (args[3]->IsObject()) { // symlink(target, path, flags, req)
625602
CHECK_EQ(args.Length(), 4);
626-
AsyncDestCall(env, args, "symlink", *path, UTF8, AfterNoArgs,
627-
uv_fs_symlink, *target, *path, flags);
603+
AsyncDestCall(env, args, "symlink", *path, path.length(), UTF8,
604+
AfterNoArgs, uv_fs_symlink, *target, *path, flags);
628605
} else { // symlink(target, path, flags)
629606
SYNC_DEST_CALL(symlink, *target, *path, *target, *path, flags)
630607
}
@@ -643,8 +620,8 @@ static void Link(const FunctionCallbackInfo<Value>& args) {
643620

644621
if (args[2]->IsObject()) { // link(src, dest, req)
645622
CHECK_EQ(args.Length(), 3);
646-
AsyncDestCall(env, args, "link", *dest, UTF8, AfterNoArgs,
647-
uv_fs_link, *src, *dest);
623+
AsyncDestCall(env, args, "link", *dest, dest.length(), UTF8,
624+
AfterNoArgs, uv_fs_link, *src, *dest);
648625
} else { // link(src, dest)
649626
SYNC_DEST_CALL(link, *src, *dest, *src, *dest)
650627
}
@@ -695,8 +672,8 @@ static void Rename(const FunctionCallbackInfo<Value>& args) {
695672

696673
if (args[2]->IsObject()) {
697674
CHECK_EQ(args.Length(), 3);
698-
AsyncDestCall(env, args, "rename", *new_path, UTF8, AfterNoArgs,
699-
uv_fs_rename, *old_path, *new_path);
675+
AsyncDestCall(env, args, "rename", *new_path, new_path.length(),
676+
UTF8, AfterNoArgs, uv_fs_rename, *old_path, *new_path);
700677
} else {
701678
SYNC_DEST_CALL(rename, *old_path, *new_path, *old_path, *new_path)
702679
}
@@ -1041,7 +1018,6 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
10411018
char* buf = nullptr;
10421019
int64_t pos;
10431020
size_t len;
1044-
FSReqWrap::Ownership ownership = FSReqWrap::COPY;
10451021

10461022
// will assign buf and len if string was external
10471023
if (!StringBytes::GetExternalParts(string,
@@ -1053,16 +1029,14 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
10531029
// StorageSize may return too large a char, so correct the actual length
10541030
// by the write size
10551031
len = StringBytes::Write(env->isolate(), buf, len, args[1], enc);
1056-
ownership = FSReqWrap::MOVE;
10571032
}
10581033
pos = GET_OFFSET(args[2]);
10591034

10601035
uv_buf_t uvbuf = uv_buf_init(const_cast<char*>(buf), len);
10611036

10621037
if (args[4]->IsObject()) {
10631038
CHECK_EQ(args.Length(), 5);
1064-
AsyncCall(env, args,
1065-
"write", UTF8, ownership, AfterInteger,
1039+
AsyncCall(env, args, "write", UTF8, AfterInteger,
10661040
uv_fs_write, fd, &uvbuf, 1, pos);
10671041
} else {
10681042
// SYNC_CALL returns on error. Make sure to always free the memory.
@@ -1071,7 +1045,7 @@ static void WriteString(const FunctionCallbackInfo<Value>& args) {
10711045
inline ~Delete() { delete[] pointer_; }
10721046
char* const pointer_;
10731047
};
1074-
Delete delete_on_return(ownership == FSReqWrap::MOVE ? buf : nullptr);
1048+
Delete delete_on_return(buf);
10751049
SYNC_CALL(write, nullptr, fd, &uvbuf, 1, pos)
10761050
return args.GetReturnValue().Set(SYNC_RESULT);
10771051
}
@@ -1373,7 +1347,7 @@ void InitFs(Local<Object> target,
13731347
Local<String> wrapString =
13741348
FIXED_ONE_BYTE_STRING(env->isolate(), "FSReqWrap");
13751349
fst->SetClassName(wrapString);
1376-
target->Set(wrapString, fst->GetFunction());
1350+
target->Set(context, wrapString, fst->GetFunction()).FromJust();
13771351
}
13781352

13791353
} // namespace fs

‎src/node_file.h‎

Lines changed: 19 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -17,60 +17,39 @@ using v8::Value;
1717

1818
namespace fs {
1919

20-
class FSReqWrap: public ReqWrap<uv_fs_t> {
20+
class FSReqWrap : public ReqWrap<uv_fs_t> {
2121
public:
22-
enum Ownership { COPY, MOVE };
22+
FSReqWrap(Environment* env, Local<Object> req)
23+
: ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP) {
24+
Wrap(object(), this);
25+
}
2326

24-
inline static FSReqWrap* New(Environment* env,
25-
Local<Object> req,
26-
const char* syscall,
27-
const char* data = nullptr,
28-
enum encoding encoding = UTF8,
29-
Ownership ownership = COPY);
27+
virtual ~FSReqWrap() {
28+
ClearWrap(object());
29+
}
3030

31-
inline void Dispose();
31+
void Init(const char* syscall,
32+
const char* data = nullptr,
33+
size_t len = 0,
34+
enum encoding encoding = UTF8);
3235

36+
virtual void FillStatsArray(const uv_stat_t* stat);
3337
virtual void Reject(Local<Value> reject);
3438
virtual void Resolve(Local<Value> value);
35-
36-
void ReleaseEarly() {
37-
if (data_ != inline_data()) {
38-
delete[] data_;
39-
data_ = nullptr;
40-
}
41-
}
39+
virtual void ResolveStat();
4240

4341
const char* syscall() const { return syscall_; }
4442
const char* data() const { return data_; }
45-
const enum encoding encoding_;
43+
enum encoding encoding() const { return encoding_; }
4644

4745
size_t self_size() const override { return sizeof(*this); }
4846

49-
protected:
50-
FSReqWrap(Environment* env,
51-
Local<Object> req,
52-
const char* syscall,
53-
const char* data,
54-
enum encoding encoding)
55-
: ReqWrap(env, req, AsyncWrap::PROVIDER_FSREQWRAP),
56-
encoding_(encoding),
57-
syscall_(syscall),
58-
data_(data) {
59-
Wrap(object(), this);
60-
}
61-
62-
virtual ~FSReqWrap() {
63-
ReleaseEarly();
64-
ClearWrap(object());
65-
}
66-
67-
void* operator new(size_t size) = delete;
68-
void* operator new(size_t size, char* storage) { return storage; }
69-
char* inline_data() { return reinterpret_cast<char*>(this + 1); }
70-
7147
private:
48+
enum encoding encoding_ = UTF8;
7249
const char* syscall_;
73-
const char* data_;
50+
51+
const char* data_ = nullptr;
52+
MaybeStackBuffer<char> buffer_;
7453

7554
DISALLOW_COPY_AND_ASSIGN(FSReqWrap);
7655
};

‎test/sequential/test-async-wrap-getasyncid.js‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,8 @@ if (common.hasCrypto) { // eslint-disable-line crypto-check
112112
const req = new FSReqWrap();
113113
req.oncomplete = () => { };
114114

115-
testUninitialized(req, 'FSReqWrap');
116-
binding.access(path.toNamespacedPath('../'), fs.F_OK, req);
117115
testInitialized(req, 'FSReqWrap');
116+
binding.access(path.toNamespacedPath('../'), fs.F_OK, req);
118117

119118
const StatWatcher = binding.StatWatcher;
120119
testInitialized(new StatWatcher(), 'StatWatcher');

0 commit comments

Comments
 (0)