Discuss backporting HandleWrap#hasRef() in a Minor

[Fishrock123]

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

cc @thealphanerd

[MylesBorins]

@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]

It presents next-to zero risk as far as I can tell. Below is the full commit chain.

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]

+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:

It presents next-to zero risk as far as I can tell. Below is the full
commit chain.

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.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#108 (comment)

[Fishrock123]

Well, I don't think I need this info on v4 anyways, so I'm willing to drop it.

[MylesBorins]

@Fishrock123 with v6 coming up for LTS soon I think we should consider not landing this on v4. Thoughts?

[Fishrock123]

¯_(ツ)_/¯

may be useful if all of async_wrap gets backported

[MylesBorins]

@Fishrock123 do you still want to pursue this for v4?

[Fishrock123]

nah