-
Notifications
You must be signed in to change notification settings - Fork 28.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
stream_base: homogenize req_wrap_obj use #10184
Conversation
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with Green CI
req_wrap->object()->Set(env->async(), True(env->isolate())); | ||
req_wrap->object()->Set(env->bytes_string(), | ||
req_wrap_obj->Set(env->async(), True(env->isolate())); | ||
req_wrap_obj->Set(env->bytes_string(), | ||
Number::New(env->isolate(), bytes)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation needs to be adjusted here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but you might as well change the one at line ~330 too.
@bnoordhuis good point, fixed it everywhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with style nits. Please be less hasty next time.
@@ -82,7 +82,7 @@ void StreamBase::AfterShutdown(ShutdownWrap* req_wrap, int status) { | |||
req_wrap_obj | |||
}; | |||
|
|||
if (req_wrap->object()->Has(env->context(), | |||
if (req_wrap_obj->Has(env->context(), | |||
env->oncomplete_string()).FromJust()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alignment. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -383,7 +382,7 @@ void StreamBase::AfterWrite(WriteWrap* req_wrap, int status) { | |||
wrap->ClearError(); | |||
} | |||
|
|||
if (req_wrap->object()->Has(env->context(), | |||
if (req_wrap_obj->Has(env->context(), | |||
env->oncomplete_string()).FromJust()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Landed in 729fecf, thank you! |
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: #10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: nodejs#10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: #10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@indutny I've gone ahead and backported to v6.x. It is not landing cleanly on v4.x though please feel free to manually backport. If it should be backported please change the label appropriately. |
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: #10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
No need to backport it, but thanks for attempt! |
@indutny should it be removed from v6.x? |
No need to remove it either. It is very minor and has neglectable behavior changes for core internals. |
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: #10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Always use `req_wrap_obj` to allow calling `req->Done()` in stream implementations. PR-URL: #10184 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
stream_base
Description of change
Always use
req_wrap_obj
to allow callingreq->Done()
in streamimplementations.