Repository navigation
Discuss backporting HandleWrap#hasRef() in a Minor #108
Description
Bah, I assume this would have been much easier if I'd not mucked up the API.
It's possible we can squash it down into less commits for a backport.
I'd like to see this since it would be useful to have this information in LTS for tooling. It's far less desirable to have to float this patch or patch in _unrefed for APIs that don't have it (e.g. child_process, whereas net and timers have this.)
This will be even more prevalent if/once anything from AsyncWrap gets backported, or even with it's existing API. Specifically, there is no way to tell if a handle will or will not keep the process open without this.
It presents next-to zero risk as far as I can tell. Below is the full commit chain.
- nodejs/node@7d8882b handle_wrap: expose {un}refed check to JS node#5834
- nodejs/node@f938ef7 src: fix -Wunused-variable compiler warning node#6129
- nodejs/node@9bb5a5e handle_wrap: IsRefed -> Unrefed, no isAlive check node#6204
- nodejs/node@cad1a62 src: use libuv's refcounting directly node#6395
- nodejs/node@a58d483 src: use libuv's refcounting directly node#6395
- nodejs/node@4282405 src: use libuv's refcounting directly node#6395
- nodejs/node@96a9439 Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" node#6546
- nodejs/node@fe4837a Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" node#6546
- Semi-depends-on nodejs/node@2c480bd src,tools: rewrite check-imports and fix linter errors node#6105
Activity
MylesBorins commentedon May 19, 2016
@Fishrock123 I'm up for landing these into staging next week and putting them into the rc to see if there are any problems
/cc @nodejs/lts @nodejs/ctc
rvagg commentedon May 20, 2016
The risk that backporting seemly non-risky changes is directly proportional to the delta between the branches involved. Simply doing the backport puts us at risk of getting things wrong and then there's the surrounding structures which may have become important for the operation of a feature without noticing and they may not be present in the older branch.
I think we'd need to see a PR for it along with a CI run and then we can make a judgement on the code. If it's particularly difficult to backport just to have a discussion then that goes back to my above point about risk.
jasnell commentedon May 20, 2016
+1 to everything @rvagg said. As the delta between master and v4 grows, we
need to become more conservative about the things we cherry pick vs
backport using a reviewed PR and need to get CI/CITGM runs going more
often. Our intuition that things "shouldn't" break is not enough of a
guarantee.
On May 19, 2016 10:48 PM, "Rod Vagg" notifications@github.com wrote:
Fishrock123 commentedon May 31, 2016
Well, I don't think I need this info on v4 anyways, so I'm willing to drop it.
MylesBorins commentedon Aug 30, 2016
@Fishrock123 with v6 coming up for LTS soon I think we should consider not landing this on v4. Thoughts?
Fishrock123 commentedon Sep 4, 2016
¯_(ツ)_/¯
may be useful if all of async_wrap gets backported
MylesBorins commentedon Nov 14, 2016
@Fishrock123 do you still want to pursue this for v4?
Fishrock123 commentedon Nov 15, 2016
nah