Skip to content

Commit 2d3f09b

Browse files
indutnyrvagg
indutny
authored and
rvagg
committed
stream_base: various improvements
Expose and use in TLSWrap an `v8::External` wrap of the `StreamBase*` pointer instead of guessing the ancestor C++ class in `node_wrap.h`. Make use of `StreamBase::Callback` structure for storing/passing both callback and context in a single object. Introduce `GetObject()` for future user-land usage, when a child class is not going to be inherited from AsyncWrap. PR-URL: #2351 Reviewed-By: Trevor Norris <[email protected]>
1 parent 3545e23 commit 2d3f09b

File tree

8 files changed

+96
-65
lines changed

8 files changed

+96
-65
lines changed

‎lib/_tls_wrap.js

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,9 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
319319
var context = options.secureContext ||
320320
options.credentials ||
321321
tls.createSecureContext();
322-
res = tls_wrap.wrap(handle, context.context, options.isServer);
322+
res = tls_wrap.wrap(handle._externalStream,
323+
context.context,
324+
options.isServer);
323325
res._parent = handle;
324326
res._parentWrap = wrap;
325327
res._secureContext = context;

‎src/env.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ namespace node {
8888
V(exponent_string, "exponent") \
8989
V(exports_string, "exports") \
9090
V(ext_key_usage_string, "ext_key_usage") \
91+
V(external_stream_string, "_externalStream") \
9192
V(family_string, "family") \
9293
V(fatal_exception_string, "_fatalException") \
9394
V(fd_string, "fd") \

‎src/node_wrap.h

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -34,21 +34,6 @@ namespace node {
3434
} \
3535
} while (0)
3636

37-
#define WITH_GENERIC_STREAM(env, obj, BODY) \
38-
do { \
39-
WITH_GENERIC_UV_STREAM(env, obj, BODY, { \
40-
if (env->tls_wrap_constructor_template().IsEmpty() == false && \
41-
env->tls_wrap_constructor_template()->HasInstance(obj)) { \
42-
TLSWrap* const wrap = Unwrap<TLSWrap>(obj); \
43-
BODY \
44-
} else if (env->jsstream_constructor_template().IsEmpty() == false && \
45-
env->jsstream_constructor_template()->HasInstance(obj)) { \
46-
JSStream* const wrap = Unwrap<JSStream>(obj); \
47-
BODY \
48-
} \
49-
}); \
50-
} while (0)
51-
5237
inline uv_stream_t* HandleToStream(Environment* env,
5338
v8::Local<v8::Object> obj) {
5439
v8::HandleScope scope(env->isolate());

‎src/stream_base-inl.h

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
namespace node {
1212

13+
using v8::External;
1314
using v8::FunctionCallbackInfo;
1415
using v8::FunctionTemplate;
1516
using v8::Handle;
@@ -36,6 +37,13 @@ void StreamBase::AddMethods(Environment* env,
3637
v8::DEFAULT,
3738
attributes);
3839

40+
t->InstanceTemplate()->SetAccessor(env->external_stream_string(),
41+
GetExternal<Base>,
42+
nullptr,
43+
env->as_external(),
44+
v8::DEFAULT,
45+
attributes);
46+
3947
env->SetProtoMethod(t, "readStart", JSMethod<Base, &StreamBase::ReadStart>);
4048
env->SetProtoMethod(t, "readStop", JSMethod<Base, &StreamBase::ReadStop>);
4149
if ((flags & kFlagNoShutdown) == 0)
@@ -72,6 +80,16 @@ void StreamBase::GetFD(Local<String> key,
7280
}
7381

7482

83+
template <class Base>
84+
void StreamBase::GetExternal(Local<String> key,
85+
const PropertyCallbackInfo<Value>& args) {
86+
StreamBase* wrap = Unwrap<Base>(args.Holder());
87+
88+
Local<External> ext = External::New(args.GetIsolate(), wrap);
89+
args.GetReturnValue().Set(ext);
90+
}
91+
92+
7593
template <class Base,
7694
int (StreamBase::*Method)(const FunctionCallbackInfo<Value>& args)>
7795
void StreamBase::JSMethod(const FunctionCallbackInfo<Value>& args) {

‎src/stream_base.cc

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -72,15 +72,14 @@ void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) {
7272

7373
// The wrap and request objects should still be there.
7474
CHECK_EQ(req_wrap->persistent().IsEmpty(), false);
75-
CHECK_EQ(wrap->GetAsyncWrap()->persistent().IsEmpty(), false);
7675

7776
HandleScope handle_scope(env->isolate());
7877
Context::Scope context_scope(env->context());
7978

8079
Local<Object> req_wrap_obj = req_wrap->object();
8180
Local<Value> argv[3] = {
8281
Integer::New(env->isolate(), status),
83-
wrap->GetAsyncWrap()->object(),
82+
wrap->GetObject(),
8483
req_wrap_obj
8584
};
8685

@@ -370,7 +369,6 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
370369

371370
// The wrap and request objects should still be there.
372371
CHECK_EQ(req_wrap->persistent().IsEmpty(), false);
373-
CHECK_EQ(wrap->GetAsyncWrap()->persistent().IsEmpty(), false);
374372

375373
// Unref handle property
376374
Local<Object> req_wrap_obj = req_wrap->object();
@@ -379,7 +377,7 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) {
379377

380378
Local<Value> argv[] = {
381379
Integer::New(env->isolate(), status),
382-
wrap->GetAsyncWrap()->object(),
380+
wrap->GetObject(),
383381
req_wrap_obj,
384382
Undefined(env->isolate())
385383
};
@@ -414,7 +412,16 @@ void StreamBase::EmitData(ssize_t nread,
414412
if (argv[2].IsEmpty())
415413
argv[2] = Undefined(env->isolate());
416414

417-
GetAsyncWrap()->MakeCallback(env->onread_string(), ARRAY_SIZE(argv), argv);
415+
AsyncWrap* async = GetAsyncWrap();
416+
if (async == nullptr) {
417+
node::MakeCallback(env,
418+
GetObject(),
419+
env->onread_string(),
420+
ARRAY_SIZE(argv),
421+
argv);
422+
} else {
423+
async->MakeCallback(env->onread_string(), ARRAY_SIZE(argv), argv);
424+
}
418425
}
419426

420427

@@ -428,6 +435,16 @@ int StreamBase::GetFD() {
428435
}
429436

430437

438+
AsyncWrap* StreamBase::GetAsyncWrap() {
439+
return nullptr;
440+
}
441+
442+
443+
Local<Object> StreamBase::GetObject() {
444+
return GetAsyncWrap()->object();
445+
}
446+
447+
431448
int StreamResource::DoTryWrite(uv_buf_t** bufs, size_t* count) {
432449
// No TryWrite by default
433450
return 0;

‎src/stream_base.h

Lines changed: 42 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -106,18 +106,31 @@ class WriteWrap: public ReqWrap<uv_write_t>,
106106

107107
class StreamResource {
108108
public:
109+
template <class T>
110+
struct Callback {
111+
Callback() : fn(nullptr), ctx(nullptr) {}
112+
Callback(T fn, void* ctx) : fn(fn), ctx(ctx) {}
113+
Callback(const Callback&) = default;
114+
115+
inline bool is_empty() { return fn == nullptr; }
116+
inline void clear() {
117+
fn = nullptr;
118+
ctx = nullptr;
119+
}
120+
121+
T fn;
122+
void* ctx;
123+
};
124+
109125
typedef void (*AfterWriteCb)(WriteWrap* w, void* ctx);
110126
typedef void (*AllocCb)(size_t size, uv_buf_t* buf, void* ctx);
111127
typedef void (*ReadCb)(ssize_t nread,
112128
const uv_buf_t* buf,
113129
uv_handle_type pending,
114130
void* ctx);
115131

116-
StreamResource() : after_write_cb_(nullptr),
117-
alloc_cb_(nullptr),
118-
read_cb_(nullptr) {
132+
StreamResource() {
119133
}
120-
121134
virtual ~StreamResource() = default;
122135

123136
virtual int DoShutdown(ShutdownWrap* req_wrap) = 0;
@@ -131,44 +144,37 @@ class StreamResource {
131144

132145
// Events
133146
inline void OnAfterWrite(WriteWrap* w) {
134-
if (after_write_cb_ != nullptr)
135-
after_write_cb_(w, after_write_ctx_);
147+
if (!after_write_cb_.is_empty())
148+
after_write_cb_.fn(w, after_write_cb_.ctx);
136149
}
137150

138151
inline void OnAlloc(size_t size, uv_buf_t* buf) {
139-
if (alloc_cb_ != nullptr)
140-
alloc_cb_(size, buf, alloc_ctx_);
152+
if (!alloc_cb_.is_empty())
153+
alloc_cb_.fn(size, buf, alloc_cb_.ctx);
141154
}
142155

143156
inline void OnRead(size_t nread,
144157
const uv_buf_t* buf,
145158
uv_handle_type pending = UV_UNKNOWN_HANDLE) {
146-
if (read_cb_ != nullptr)
147-
read_cb_(nread, buf, pending, read_ctx_);
159+
if (!read_cb_.is_empty())
160+
read_cb_.fn(nread, buf, pending, read_cb_.ctx);
148161
}
149162

150-
inline void set_after_write_cb(AfterWriteCb cb, void* ctx) {
151-
after_write_ctx_ = ctx;
152-
after_write_cb_ = cb;
163+
inline void set_after_write_cb(Callback<AfterWriteCb> c) {
164+
after_write_cb_ = c;
153165
}
154166

155-
inline void set_alloc_cb(AllocCb cb, void* ctx) {
156-
alloc_cb_ = cb;
157-
alloc_ctx_ = ctx;
158-
}
167+
inline void set_alloc_cb(Callback<AllocCb> c) { alloc_cb_ = c; }
168+
inline void set_read_cb(Callback<ReadCb> c) { read_cb_ = c; }
159169

160-
inline void set_read_cb(ReadCb cb, void* ctx) {
161-
read_cb_ = cb;
162-
read_ctx_ = ctx;
163-
}
170+
inline Callback<AfterWriteCb> after_write_cb() { return after_write_cb_; }
171+
inline Callback<AllocCb> alloc_cb() { return alloc_cb_; }
172+
inline Callback<ReadCb> read_cb() { return read_cb_; }
164173

165174
private:
166-
AfterWriteCb after_write_cb_;
167-
void* after_write_ctx_;
168-
AllocCb alloc_cb_;
169-
void* alloc_ctx_;
170-
ReadCb read_cb_;
171-
void* read_ctx_;
175+
Callback<AfterWriteCb> after_write_cb_;
176+
Callback<AllocCb> alloc_cb_;
177+
Callback<ReadCb> read_cb_;
172178
};
173179

174180
class StreamBase : public StreamResource {
@@ -211,7 +217,9 @@ class StreamBase : public StreamResource {
211217

212218
virtual ~StreamBase() = default;
213219

214-
virtual AsyncWrap* GetAsyncWrap() = 0;
220+
// One of these must be implemented
221+
virtual AsyncWrap* GetAsyncWrap();
222+
virtual v8::Local<v8::Object> GetObject();
215223

216224
// Libuv callbacks
217225
static void AfterShutdown(ShutdownWrap* req, int status);
@@ -227,8 +235,12 @@ class StreamBase : public StreamResource {
227235
int WriteString(const v8::FunctionCallbackInfo<v8::Value>& args);
228236

229237
template <class Base>
230-
static void GetFD(v8::Local<v8::String>,
231-
const v8::PropertyCallbackInfo<v8::Value>&);
238+
static void GetFD(v8::Local<v8::String> key,
239+
const v8::PropertyCallbackInfo<v8::Value>& args);
240+
241+
template <class Base>
242+
static void GetExternal(v8::Local<v8::String> key,
243+
const v8::PropertyCallbackInfo<v8::Value>& args);
232244

233245
template <class Base,
234246
int (StreamBase::*Method)( // NOLINT(whitespace/parens)

‎src/stream_wrap.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ StreamWrap::StreamWrap(Environment* env,
7474
parent),
7575
StreamBase(env),
7676
stream_(stream) {
77-
set_after_write_cb(OnAfterWriteImpl, this);
78-
set_alloc_cb(OnAllocImpl, this);
79-
set_read_cb(OnReadImpl, this);
77+
set_after_write_cb({ OnAfterWriteImpl, this });
78+
set_alloc_cb({ OnAllocImpl, this });
79+
set_read_cb({ OnReadImpl, this });
8080
}
8181

8282

‎src/tls_wrap.cc

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
#include "node_crypto_bio.h" // NodeBIO
77
#include "node_crypto_clienthello.h" // ClientHelloParser
88
#include "node_crypto_clienthello-inl.h"
9-
#include "node_wrap.h" // WithGenericStream
109
#include "node_counters.h"
1110
#include "node_internals.h"
1211
#include "stream_base.h"
@@ -63,12 +62,12 @@ TLSWrap::TLSWrap(Environment* env,
6362
SSL_CTX_sess_set_new_cb(sc_->ctx_, SSLWrap<TLSWrap>::NewSessionCallback);
6463

6564
stream_->Consume();
66-
stream_->set_after_write_cb(OnAfterWriteImpl, this);
67-
stream_->set_alloc_cb(OnAllocImpl, this);
68-
stream_->set_read_cb(OnReadImpl, this);
65+
stream_->set_after_write_cb({ OnAfterWriteImpl, this });
66+
stream_->set_alloc_cb({ OnAllocImpl, this });
67+
stream_->set_read_cb({ OnReadImpl, this });
6968

70-
set_alloc_cb(OnAllocSelf, this);
71-
set_read_cb(OnReadSelf, this);
69+
set_alloc_cb({ OnAllocSelf, this });
70+
set_read_cb({ OnReadSelf, this });
7271

7372
InitSSL();
7473
}
@@ -177,15 +176,12 @@ void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
177176
if (args.Length() < 3 || !args[2]->IsBoolean())
178177
return env->ThrowTypeError("Third argument should be boolean");
179178

180-
Local<Object> stream_obj = args[0].As<Object>();
179+
Local<External> stream_obj = args[0].As<External>();
181180
Local<Object> sc = args[1].As<Object>();
182181
Kind kind = args[2]->IsTrue() ? SSLWrap<TLSWrap>::kServer :
183182
SSLWrap<TLSWrap>::kClient;
184183

185-
StreamBase* stream = nullptr;
186-
WITH_GENERIC_STREAM(env, stream_obj, {
187-
stream = wrap;
188-
});
184+
StreamBase* stream = static_cast<StreamBase*>(stream_obj->Value());
189185
CHECK_NE(stream, nullptr);
190186

191187
TLSWrap* res = new TLSWrap(env, kind, stream, Unwrap<SecureContext>(sc));

0 commit comments

Comments
 (0)