src: use libuv's refcounting directly #6395

bnoordhuis · 2016-04-26T12:04:53Z

The first commit is cleanup:

Don't implement an additional reference counting scheme on top of libuv.
Libuv is the canonical source for that information so use it directly.

The second one is what I think the logic should be:

This also updates the tests to expect that a closed handle has no
reference count.

R=@Fishrock123

CI: https://ci.nodejs.org/job/node-test-pull-request/2398/

addaleax · 2016-04-26T12:15:22Z

Nice, LGTM!

cjihrig · 2016-04-26T13:32:56Z

LGTM

Fishrock123 · 2016-04-26T14:01:59Z

This still has the same behavior though?

i.e. IsAlive is required to get the unref flag from a handle in case it is gone?

Should we just track this in JS?

NOT lgtm since this will interfere with likely reverts of my commits.

addaleax · 2016-04-26T14:16:39Z

@Fishrock123 fwiw, #6381 passes when replayed on top of this

bnoordhuis · 2016-04-26T15:11:35Z

Should we just track this in JS?

I'm not sure what 'this' refers to. The reference count, liveliness, or something else?

jasnell · 2016-04-26T15:43:20Z

LGTM. Should this go into v6?

bnoordhuis · 2016-04-26T16:16:21Z

Well... #6401 might block that.

Don't implement an additional reference counting scheme on top of libuv. Libuv is the canonical source for that information so use it directly. PR-URL: nodejs#6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This also updates the tests to expect that a closed handle has no reference count. PR-URL: nodejs#6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Verify that a second call to handle.close() is a no-op. PR-URL: nodejs#6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis · 2016-04-27T14:33:24Z

Seeing how the reverts never landed (#6401 (comment)), I went ahead and landed this in 23818c6...4282405.

Fishrock123 · 2016-04-27T14:48:52Z

That's fine; but this is also a behavior breaker and will make fixing my stupid API problems even more complex.

This reverts commit 9bb5a5e. Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Fixes: nodejs#6381 Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

This reverts commit 9bb5a5e. This API is not suitable because it depended on being able to potentially access the handle's flag after the handle was already cleaned up. Since this is not actually possible (obviously, oops) this newer API no longer makes much sense, and the older API is more suitable. API comparison: IsRefed -> Has a strong reference AND is alive. (Deterministic) Unrefed -> Has a weak reference OR is dead. (Less deterministic) Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Fixes: nodejs#6381 PR-URL: nodejs#6546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

Rename slightly to HasRef() at bnoordhuis’ request. Better reflects what we actually do for this check. Refs: nodejs#6395 Refs: nodejs#6204 Refs: nodejs#6401 Refs: nodejs#6382 Refs: nodejs#6381 PR-URL: nodejs#6546 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Don't implement an additional reference counting scheme on top of libuv. Libuv is the canonical source for that information so use it directly. PR-URL: #6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This also updates the tests to expect that a closed handle has no reference count. PR-URL: #6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Verify that a second call to handle.close() is a no-op. PR-URL: #6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This reverts commit 9bb5a5e. This API is not suitable because it depended on being able to potentially access the handle's flag after the handle was already cleaned up. Since this is not actually possible (obviously, oops) this newer API no longer makes much sense, and the older API is more suitable. API comparison: IsRefed -> Has a strong reference AND is alive. (Deterministic) Unrefed -> Has a weak reference OR is dead. (Less deterministic) Refs: #6395 Refs: #6204 Refs: #6401 Refs: #6382 Fixes: #6381 PR-URL: #6546 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Conflicts: src/handle_wrap.cc test/parallel/test-handle-wrap-isrefed-tty.js test/parallel/test-handle-wrap-isrefed.js

Rename slightly to HasRef() at bnoordhuis’ request. Better reflects what we actually do for this check. Refs: #6395 Refs: #6204 Refs: #6401 Refs: #6382 Refs: #6381 PR-URL: #6546 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Trevor Norris <trev.norris@gmail.com>

MylesBorins · 2016-06-01T23:44:36Z

I'm setting this as don't land... @Fishrock123 / @bnoordhuis please let me know if that is incorrect

Fishrock123 · 2016-06-02T16:23:22Z

Hmmmm, seems fine unless @bnoordhuis Thinks it's important to backport. May need to be manual because this depended on my handle wrap additions.

bnoordhuis · 2016-06-02T17:20:18Z

I'm okay with having it back-ported (as long as I don't have to do the work - hah!) but it's not strictly necessary from a 'bug fixes only' perspective.

MylesBorins · 2016-06-02T17:25:19Z

I'll put it on watch and see if it lands nicely... if it doesn't we just won't do it 😄

MylesBorins · 2016-06-02T23:41:01Z

@bnoordhuis this does not land cleanly and I've added the dont-land label. Please feel free to revisit this if you like

bnoordhuis added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 26, 2016

bnoordhuis added 3 commits April 27, 2016 16:26

test: check that 2nd handle.close() call is a nop

4282405

Verify that a second call to handle.close() is a no-op. PR-URL: nodejs#6395 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

bnoordhuis force-pushed the handlewrap-refcounting branch from fd087d5 to 4282405 Compare April 27, 2016 14:32

bnoordhuis closed this Apr 27, 2016

bnoordhuis deleted the handlewrap-refcounting branch April 27, 2016 14:32

bnoordhuis merged commit 4282405 into nodejs:master Apr 27, 2016

bnoordhuis added dont-land-on-v5.x labels Apr 27, 2016

Fishrock123 mentioned this pull request May 3, 2016

Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check" #6546

Merged

3 tasks

Fishrock123 removed the dont-land-on-v4.x label May 17, 2016

Fishrock123 removed dont-land-on-v5.x labels May 17, 2016

Fishrock123 mentioned this pull request May 17, 2016

handle_wrap: expose {un}refed check to JS #5834

Merged

2 tasks

evanlucas mentioned this pull request May 17, 2016

2016-05-17, Version 6.2.0 (Stable) #6810

Merged

Fishrock123 mentioned this pull request May 19, 2016

Discuss backporting HandleWrap#hasRef() in a Minor nodejs/Release#108

Closed

MylesBorins added the dont-land-on-v4.x label Jun 1, 2016

MylesBorins added lts-watch-v4.x and removed dont-land-on-v4.x labels Jun 2, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Jun 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use libuv's refcounting directly #6395

src: use libuv's refcounting directly #6395

bnoordhuis commented Apr 26, 2016 •

edited

Loading

addaleax commented Apr 26, 2016

cjihrig commented Apr 26, 2016

Fishrock123 commented Apr 26, 2016

addaleax commented Apr 26, 2016

bnoordhuis commented Apr 26, 2016

jasnell commented Apr 26, 2016

bnoordhuis commented Apr 26, 2016

bnoordhuis commented Apr 27, 2016

Fishrock123 commented Apr 27, 2016

MylesBorins commented Jun 1, 2016

Fishrock123 commented Jun 2, 2016

bnoordhuis commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

src: use libuv's refcounting directly #6395

src: use libuv's refcounting directly #6395

Conversation

bnoordhuis commented Apr 26, 2016 • edited Loading

addaleax commented Apr 26, 2016

cjihrig commented Apr 26, 2016

Fishrock123 commented Apr 26, 2016

addaleax commented Apr 26, 2016

bnoordhuis commented Apr 26, 2016

jasnell commented Apr 26, 2016

bnoordhuis commented Apr 26, 2016

bnoordhuis commented Apr 27, 2016

Fishrock123 commented Apr 27, 2016

MylesBorins commented Jun 1, 2016

Fishrock123 commented Jun 2, 2016

bnoordhuis commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

MylesBorins commented Jun 2, 2016

bnoordhuis commented Apr 26, 2016 •

edited

Loading