FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: use libuv's refcounting directly by bnoordhuis · Pull Request #6395 · nodejs/node · GitHub

/ node Public

src: use libuv's refcounting directly - #6395

Merged
bnoordhuis merged 3 commits into
nodejs:masterfrom
bnoordhuis:handlewrap-refcounting
Apr 27, 2016
ConversationCommits (3)ChecksFiles changed
Merged

src: use libuv's refcounting directly #6395
bnoordhuis merged 3 commits into
nodejs:masterfrom
bnoordhuis:handlewrap-refcounting

Conversation

bnoordhuis commented Apr 26, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

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/

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

addaleax commented Apr 26, 2016

Copy link
Copy Markdown
Member

Nice, LGTM!

cjihrig commented Apr 26, 2016

Copy link
Copy Markdown
Contributor

LGTM

Fishrock123 commented Apr 26, 2016

Copy link
Copy Markdown
Contributor

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 commented Apr 26, 2016

Copy link
Copy Markdown
Member

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

bnoordhuis commented Apr 26, 2016

Copy link
Copy Markdown
Member Author

Should we just track this in JS?

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

jasnell commented Apr 26, 2016

Copy link
Copy Markdown
Member

LGTM. Should this go into v6?

bnoordhuis commented Apr 26, 2016

Copy link
Copy Markdown
Member Author

Well... #6401 might block that.

src: use libuv's refcounting directly
cad1a62 
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>
src: simplify handlewrap state tracking logic
a58d483 
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>
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 commented Apr 27, 2016

Copy link
Copy Markdown
Member Author

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

Fishrock123 commented Apr 27, 2016

Copy link
Copy Markdown
Contributor

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

Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 3, 2016
Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check"
e19c5dd 
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
Fishrock123 mentioned this pull request May 3, 2016
Merged
3 tasks
Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 11, 2016
Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check"
96a9439 
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
Fishrock123 added a commit to Fishrock123/node that referenced this pull request May 11, 2016
handle_wrap: IsRefed() -> HasRef()
fe4837a 
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>
Fishrock123 mentioned this pull request May 17, 2016
Merged
2 tasks
evanlucas pushed a commit that referenced this pull request May 17, 2016
src: use libuv's refcounting directly
965274d 
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>
evanlucas pushed a commit that referenced this pull request May 17, 2016
src: simplify handlewrap state tracking logic
bbf3b3e 
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>
evanlucas pushed a commit that referenced this pull request May 17, 2016
test: check that 2nd handle.close() call is a nop
8a7e68f 
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>
evanlucas pushed a commit that referenced this pull request May 17, 2016
Revert "handle_wrap: IsRefed -> Unrefed, no isAlive check"
cc2af79 
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
evanlucas pushed a commit that referenced this pull request May 17, 2016
handle_wrap: IsRefed() -> HasRef()
f31a5ec 
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>
evanlucas mentioned this pull request May 17, 2016
Merged
Fishrock123 mentioned this pull request May 19, 2016
Closed

MylesBorins commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

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

Fishrock123 commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

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 commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

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 commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL