stream_wrap: reference handle before uv_write2 #4663

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Owner

indutny commented Jan 25, 2013

Before sending handle to another process using uv_write2(), it should be
referenced to prevent it from being GCed before AfterWrite() will be
called.

see #4599

/cc @bnoordhuis

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Jan 25, 2013

src/stream_wrap.cc
@@ -411,6 +413,10 @@ void StreamWrap::OnRead2(uv_pipe_t* handle, ssize_t nread, uv_buf_t buf,
StreamWrap* send_stream_wrap = static_cast<StreamWrap*>(
send_stream_obj->GetAlignedPointerFromInternalField(0));
send_stream = send_stream_wrap->GetStream();
+
+ // Reference StreamWrap instance to prevent it from being garbage
+ // collected before `AfterWrite` will be called.
+ req_wrap->object_->Set(handle_sym, send_stream_obj);
@bnoordhuis

bnoordhuis Jan 25, 2013

Owner

Maybe lazily initialize handle_sym, it's not used in the common case.

Also, maybe clear the ref afterwards?

@indutny

indutny Jan 25, 2013

Owner

after callback? yeah, sounds like a plan.

@indutny indutny stream_wrap: reference handle before uv_write2
Before sending handle to another process using uv_write2(), it should be
referenced to prevent it from being GCed before AfterWrite() will be
called.

see #4599
688f861

@bnoordhuis bnoordhuis commented on the diff Jan 26, 2013

src/stream_wrap.cc
@@ -411,6 +412,13 @@ void StreamWrap::OnRead2(uv_pipe_t* handle, ssize_t nread, uv_buf_t buf,
StreamWrap* send_stream_wrap = static_cast<StreamWrap*>(
send_stream_obj->GetAlignedPointerFromInternalField(0));
send_stream = send_stream_wrap->GetStream();
+
+ // Reference StreamWrap instance to prevent it from being garbage
+ // collected before `AfterWrite` will be called.
@bnoordhuis

bnoordhuis Jan 26, 2013

Owner

s/will be/is/

@bnoordhuis bnoordhuis commented on the diff Jan 26, 2013

src/stream_wrap.cc
@@ -468,6 +476,9 @@ void StreamWrap::AfterWrite(uv_write_t* req, int status) {
assert(req_wrap->object_.IsEmpty() == false);
assert(wrap->object_.IsEmpty() == false);
+ // Unref handle propery
@bnoordhuis

bnoordhuis Jan 26, 2013

Owner

s/propery/properly/ (or 'property')?

Owner

indutny commented Jan 26, 2013

Thanks, landed with fixes in 99f0b02

indutny closed this Feb 7, 2013

indutny deleted the indutny:fix-referencing-stream branch Feb 7, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment