From da5ad92ab221e45648c871cf488e0699b5708288 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 7 Aug 2013 17:14:16 +0200 Subject: [PATCH] stream_wrap: use getters, not direct field access Hide member fields behind getters. Make the fields themselves const in the sense that the pointer is non-assignable - the pointed to object remains mutable. Makes reasoning about lifecycle and mutability a little easier. --- src/stream_wrap.cc | 101 +++++++++++++++++++++++---------------------- src/stream_wrap.h | 45 ++++++++++++-------- src/tls_wrap.cc | 10 ++--- 3 files changed, 84 insertions(+), 72 deletions(-) diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 17c8b89fae..10e70bfaa6 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -76,9 +76,9 @@ void StreamWrap::Initialize(Handle target) { StreamWrap::StreamWrap(Handle object, uv_stream_t* stream) : HandleWrap(object, reinterpret_cast(stream)), - default_callbacks_(this) { - stream_ = stream; - callbacks_ = &default_callbacks_; + stream_(stream), + default_callbacks_(this), + callbacks_(&default_callbacks_) { } @@ -87,7 +87,9 @@ void StreamWrap::GetFD(Local, const PropertyCallbackInfo& args) { HandleScope scope(node_isolate); UNWRAP_NO_ABORT(StreamWrap) int fd = -1; - if (wrap != NULL && wrap->stream_ != NULL) fd = wrap->stream_->io_watcher.fd; + if (wrap != NULL && wrap->stream() != NULL) { + fd = wrap->stream()->io_watcher.fd; + } args.GetReturnValue().Set(fd); #endif } @@ -96,7 +98,7 @@ void StreamWrap::GetFD(Local, const PropertyCallbackInfo& args) { void StreamWrap::UpdateWriteQueueSize() { HandleScope scope(node_isolate); object()->Set(write_queue_size_sym, - Integer::New(stream_->write_queue_size, node_isolate)); + Integer::New(stream()->write_queue_size, node_isolate)); } @@ -105,13 +107,13 @@ void StreamWrap::ReadStart(const FunctionCallbackInfo& args) { UNWRAP(StreamWrap) - bool ipc_pipe = wrap->stream_->type == UV_NAMED_PIPE && - reinterpret_cast(wrap->stream_)->ipc; + bool ipc_pipe = wrap->stream()->type == UV_NAMED_PIPE && + reinterpret_cast(wrap->stream())->ipc; int err; if (ipc_pipe) { - err = uv_read2_start(wrap->stream_, OnAlloc, OnRead2); + err = uv_read2_start(wrap->stream(), OnAlloc, OnRead2); } else { - err = uv_read_start(wrap->stream_, OnAlloc, OnRead); + err = uv_read_start(wrap->stream(), OnAlloc, OnRead); } args.GetReturnValue().Set(err); @@ -123,16 +125,15 @@ void StreamWrap::ReadStop(const FunctionCallbackInfo& args) { UNWRAP(StreamWrap) - int err = uv_read_stop(wrap->stream_); + int err = uv_read_stop(wrap->stream()); args.GetReturnValue().Set(err); } uv_buf_t StreamWrap::OnAlloc(uv_handle_t* handle, size_t suggested_size) { StreamWrap* wrap = static_cast(handle->data); - assert(wrap->stream_ == reinterpret_cast(handle)); - - return wrap->callbacks_->DoAlloc(handle, suggested_size); + assert(wrap->stream() == reinterpret_cast(handle)); + return wrap->callbacks()->DoAlloc(handle, suggested_size); } @@ -171,14 +172,14 @@ void StreamWrap::OnReadCommon(uv_stream_t* handle, assert(wrap->persistent().IsEmpty() == false); if (nread > 0) { - if (wrap->stream_->type == UV_TCP) { + if (wrap->stream()->type == UV_TCP) { NODE_COUNT_NET_BYTES_RECV(nread); - } else if (wrap->stream_->type == UV_NAMED_PIPE) { + } else if (wrap->stream()->type == UV_NAMED_PIPE) { NODE_COUNT_PIPE_BYTES_RECV(nread); } } - wrap->callbacks_->DoRead(handle, nread, buf, pending); + wrap->callbacks()->DoRead(handle, nread, buf, pending); } @@ -224,11 +225,11 @@ void StreamWrap::WriteBuffer(const FunctionCallbackInfo& args) { uv_buf_t buf; WriteBuffer(buf_obj, &buf); - int err = wrap->callbacks_->DoWrite(req_wrap, - &buf, - 1, - NULL, - StreamWrap::AfterWrite); + int err = wrap->callbacks()->DoWrite(req_wrap, + &buf, + 1, + NULL, + StreamWrap::AfterWrite); req_wrap->Dispatched(); req_wrap_obj->Set(bytes_sym, Integer::NewFromUnsigned(length, node_isolate)); @@ -284,15 +285,15 @@ void StreamWrap::WriteStringImpl(const FunctionCallbackInfo& args) { buf.base = data; buf.len = data_size; - bool ipc_pipe = wrap->stream_->type == UV_NAMED_PIPE && - reinterpret_cast(wrap->stream_)->ipc; + bool ipc_pipe = wrap->stream()->type == UV_NAMED_PIPE && + reinterpret_cast(wrap->stream())->ipc; if (!ipc_pipe) { - err = wrap->callbacks_->DoWrite(req_wrap, - &buf, - 1, - NULL, - StreamWrap::AfterWrite); + err = wrap->callbacks()->DoWrite(req_wrap, + &buf, + 1, + NULL, + StreamWrap::AfterWrite); } else { uv_handle_t* send_handle = NULL; @@ -312,11 +313,11 @@ void StreamWrap::WriteStringImpl(const FunctionCallbackInfo& args) { req_wrap->object()->Set(handle_sym, send_handle_obj); } - err = wrap->callbacks_->DoWrite(req_wrap, - &buf, - 1, - reinterpret_cast(send_handle), - StreamWrap::AfterWrite); + err = wrap->callbacks()->DoWrite(req_wrap, + &buf, + 1, + reinterpret_cast(send_handle), + StreamWrap::AfterWrite); } req_wrap->Dispatched(); @@ -407,11 +408,11 @@ void StreamWrap::Writev(const FunctionCallbackInfo& args) { bytes += str_size; } - int err = wrap->callbacks_->DoWrite(req_wrap, - bufs, - count, - NULL, - StreamWrap::AfterWrite); + int err = wrap->callbacks()->DoWrite(req_wrap, + bufs, + count, + NULL, + StreamWrap::AfterWrite); // Deallocate space if (bufs != bufs_) @@ -446,7 +447,7 @@ void StreamWrap::WriteUcs2String(const FunctionCallbackInfo& args) { void StreamWrap::AfterWrite(uv_write_t* req, int status) { WriteWrap* req_wrap = container_of(req, WriteWrap, req_); - StreamWrap* wrap = req_wrap->wrap_; + StreamWrap* wrap = req_wrap->wrap(); HandleScope scope(node_isolate); @@ -460,7 +461,7 @@ void StreamWrap::AfterWrite(uv_write_t* req, int status) { req_wrap_obj->Delete(handle_sym); } - wrap->callbacks_->AfterWrite(req_wrap); + wrap->callbacks()->AfterWrite(req_wrap); Local argv[] = { Integer::New(status, node_isolate), @@ -484,7 +485,7 @@ void StreamWrap::Shutdown(const FunctionCallbackInfo& args) { Local req_wrap_obj = args[0].As(); ShutdownWrap* req_wrap = new ShutdownWrap(req_wrap_obj); - int err = wrap->callbacks_->DoShutdown(req_wrap, AfterShutdown); + int err = wrap->callbacks()->DoShutdown(req_wrap, AfterShutdown); req_wrap->Dispatched(); if (err) delete req_wrap; args.GetReturnValue().Set(err); @@ -521,30 +522,30 @@ int StreamWrapCallbacks::DoWrite(WriteWrap* w, uv_write_cb cb) { int r; if (send_handle == NULL) { - r = uv_write(&w->req_, wrap_->stream_, bufs, count, cb); + r = uv_write(&w->req_, wrap()->stream(), bufs, count, cb); } else { - r = uv_write2(&w->req_, wrap_->stream_, bufs, count, send_handle, cb); + r = uv_write2(&w->req_, wrap()->stream(), bufs, count, send_handle, cb); } if (!r) { size_t bytes = 0; for (size_t i = 0; i < count; i++) bytes += bufs[i].len; - if (wrap_->stream_->type == UV_TCP) { + if (wrap()->stream()->type == UV_TCP) { NODE_COUNT_NET_BYTES_SENT(bytes); - } else if (wrap_->stream_->type == UV_NAMED_PIPE) { + } else if (wrap()->stream()->type == UV_NAMED_PIPE) { NODE_COUNT_PIPE_BYTES_SENT(bytes); } } - wrap_->UpdateWriteQueueSize(); + wrap()->UpdateWriteQueueSize(); return r; } void StreamWrapCallbacks::AfterWrite(WriteWrap* w) { - wrap_->UpdateWriteQueueSize(); + wrap()->UpdateWriteQueueSize(); } @@ -603,17 +604,17 @@ void StreamWrapCallbacks::DoRead(uv_stream_t* handle, argv[2] = pending_obj; } - MakeCallback(wrap_->object(), onread_sym, ARRAY_SIZE(argv), argv); + MakeCallback(wrap()->object(), onread_sym, ARRAY_SIZE(argv), argv); } int StreamWrapCallbacks::DoShutdown(ShutdownWrap* req_wrap, uv_shutdown_cb cb) { - return uv_shutdown(&req_wrap->req_, wrap_->stream_, cb); + return uv_shutdown(&req_wrap->req_, wrap()->stream(), cb); } Handle StreamWrapCallbacks::Self() { - return wrap_->object(); + return wrap()->object(); } } // namespace node diff --git a/src/stream_wrap.h b/src/stream_wrap.h index 98d22cbfad..25aff8b8d3 100644 --- a/src/stream_wrap.h +++ b/src/stream_wrap.h @@ -37,9 +37,9 @@ typedef class ReqWrap ShutdownWrap; class WriteWrap: public ReqWrap { public: - explicit WriteWrap(v8::Local obj, StreamWrap* wrap) - : ReqWrap(obj) { - wrap_ = wrap; + WriteWrap(v8::Local obj, StreamWrap* wrap) + : ReqWrap(obj) + , wrap_(wrap) { } void* operator new(size_t size, char* storage) { return storage; } @@ -48,13 +48,17 @@ class WriteWrap: public ReqWrap { // we don't use exceptions in node. void operator delete(void* ptr, char* storage) { assert(0); } - StreamWrap* wrap_; + inline StreamWrap* wrap() const { + return wrap_; + } - protected: + private: // People should not be using the non-placement new and delete operator on a // WriteWrap. Ensure this never happens. void* operator new(size_t size) { assert(0); } void operator delete(void* ptr) { assert(0); } + + StreamWrap* const wrap_; }; // Overridable callbacks' types @@ -63,7 +67,7 @@ class StreamWrapCallbacks { explicit StreamWrapCallbacks(StreamWrap* wrap) : wrap_(wrap) { } - explicit StreamWrapCallbacks(StreamWrapCallbacks* old) : wrap_(old->wrap_) { + explicit StreamWrapCallbacks(StreamWrapCallbacks* old) : wrap_(old->wrap()) { } virtual ~StreamWrapCallbacks() { @@ -85,13 +89,16 @@ class StreamWrapCallbacks { v8::Handle Self(); protected: - StreamWrap* wrap_; + inline StreamWrap* wrap() const { + return wrap_; + } + + private: + StreamWrap* const wrap_; }; class StreamWrap : public HandleWrap { public: - uv_stream_t* GetStream() { return stream_; } - void OverrideCallbacks(StreamWrapCallbacks* callbacks) { StreamWrapCallbacks* old = callbacks_; callbacks_ = callbacks; @@ -99,10 +106,6 @@ class StreamWrap : public HandleWrap { delete old; } - StreamWrapCallbacks* GetCallbacks() { - return callbacks_; - } - static void Initialize(v8::Handle target); static void GetFD(v8::Local, @@ -119,19 +122,26 @@ class StreamWrap : public HandleWrap { static void WriteUtf8String(const v8::FunctionCallbackInfo& args); static void WriteUcs2String(const v8::FunctionCallbackInfo& args); - // Overridable callbacks - StreamWrapCallbacks* callbacks_; + inline StreamWrapCallbacks* callbacks() const { + return callbacks_; + } + + inline uv_stream_t* stream() const { + return stream_; + } protected: static size_t WriteBuffer(v8::Handle val, uv_buf_t* buf); StreamWrap(v8::Handle object, uv_stream_t* stream); + ~StreamWrap() { if (callbacks_ != &default_callbacks_) { delete callbacks_; callbacks_ = NULL; } } + void StateChange() { } void UpdateWriteQueueSize(); @@ -150,9 +160,10 @@ class StreamWrap : public HandleWrap { template static void WriteStringImpl(const v8::FunctionCallbackInfo& args); - uv_stream_t* stream_; - + uv_stream_t* const stream_; StreamWrapCallbacks default_callbacks_; + StreamWrapCallbacks* callbacks_; // Overridable callbacks + friend class StreamWrapCallbacks; }; diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index b3172a1ac1..66dfd6e4a0 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -316,7 +316,7 @@ void TLSCallbacks::Wrap(const FunctionCallbackInfo& args) { TLSCallbacks* callbacks = NULL; WITH_GENERIC_STREAM(stream, { - callbacks = new TLSCallbacks(kind, sc, wrap->GetCallbacks()); + callbacks = new TLSCallbacks(kind, sc, wrap->callbacks()); wrap->OverrideCallbacks(callbacks); }); @@ -394,13 +394,13 @@ void TLSCallbacks::EncOut() { write_req_.data = this; uv_buf_t buf = uv_buf_init(data, write_size_); - int r = uv_write(&write_req_, wrap_->GetStream(), &buf, 1, EncOutCb); + int r = uv_write(&write_req_, wrap()->stream(), &buf, 1, EncOutCb); // Ignore errors, this should be already handled in js if (!r) { - if (wrap_->GetStream()->type == UV_TCP) { + if (wrap()->stream()->type == UV_TCP) { NODE_COUNT_NET_BYTES_SENT(write_size_); - } else if (wrap_->GetStream()->type == UV_NAMED_PIPE) { + } else if (wrap()->stream()->type == UV_NAMED_PIPE) { NODE_COUNT_PIPE_BYTES_SENT(write_size_); } } @@ -560,7 +560,7 @@ int TLSCallbacks::DoWrite(WriteWrap* w, // However if there any data that should be written to socket, // callback should not be invoked immediately if (BIO_pending(enc_out_) == 0) - return uv_write(&w->req_, wrap_->GetStream(), bufs, count, cb); + return uv_write(&w->req_, wrap()->stream(), bufs, count, cb); } QUEUE_INSERT_TAIL(&write_item_queue_, &wi->member_);