Skip to content
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

Fix SYNC_CALL/SYNC_DEST_CALL in node_file.cc #207

Closed
piscisaureus opened this issue Dec 24, 2014 · 4 comments
Closed

Fix SYNC_CALL/SYNC_DEST_CALL in node_file.cc #207

piscisaureus opened this issue Dec 24, 2014 · 4 comments
Assignees

Comments

@piscisaureus
Copy link
Contributor

f16edd2

Something's odd here:

  • This commit breaks symmetry between SYNC_CALL and ASYNC_CALL
  • It causes fs.symlink errors to show the wrong path
@piscisaureus piscisaureus self-assigned this Dec 24, 2014
@vkurchatkin
Copy link
Contributor

@piscisaureus is it still an issue? f6556b6 was supposed to fix that

@piscisaureus piscisaureus changed the title Review f16edd2632930e3fbfead4d6d52eeac87824f1a6 Fix SYNC_CALL/SYNC_DEST_CALL in node_file.cc Dec 24, 2014
@piscisaureus
Copy link
Contributor Author

@vkurchatkin Yes, still an issue.

(A)SYNC_DEST_CALL is not supposed to be used for symlink. That's because the first argument to symlink() isn't an actual path.

@caitp
Copy link
Contributor

caitp commented Jan 11, 2015

I was thinking:

Using macros serves some purpose (support for multiple types of arguments), however it has some problems: makes things difficult to debug, a bit overspecialized in some ways, too.

So, how about using lambda expressions (in addition to macros, or else replacing macros with template functions)?

For instance, instead of having specialized macros, you don't need variadic macros if the function itself is called using a lambda expression, and error handling could also be handled with lambdas. The compiler can inline these and eliminate dead code, and it should be much easier to debug.

Variadic macros still have limited toolchain support, and make debugging much harder, so it seems like it could be a good tradeoff

(I realize this is unrelated to the issue here, but same code area, wanted thoughts on it since it might harm toolchain support a bit. We still aren't using lambdas in v8)

@indutny
Copy link
Member

indutny commented Jan 11, 2015

I'm +1 for template functions.

piscisaureus added a commit to piscisaureus/node2 that referenced this issue Jan 31, 2015
* Include a description for the error message
* For rename, link, and symlink, include both the source and destination
  path in the error message.
* Expose the destination path as the `dest` property on the error object.
* Fix a bug where `ThrowUVException()` would incorrectly delegate to
  `Environment::TrowErrnoException()`.

API impact:
* Adds an extra overload for node::UVException() which takes 6
  arguments.

PR: nodejs#675
Fixes: nodejs#207
Closes: nodejs#293
Reviewed-by: Ben Noordhuis <[email protected]>
jasongin pushed a commit to jasongin/nodejs that referenced this issue Mar 29, 2017
* Refcount for v8impl::Reference should be unsigned

Add ```Reference::RefCount()``` and check the refcount value before trying to call ```Reference::Unref()``` to avoid underflowing the refcount. This allows us to continue returning an error from ```napi_reference_unref``` if it is called with a reference that already has refcount set to zero.

* Add check for _refcount == 0 in Reference::Unref to avoid an underflow
boingoing added a commit to boingoing/node that referenced this issue Apr 6, 2017
* Refcount for v8impl::Reference should be unsigned

Add ```Reference::RefCount()``` and check the refcount value before trying to call ```Reference::Unref()``` to avoid underflowing the refcount. This allows us to continue returning an error from ```napi_reference_unref``` if it is called with a reference that already has refcount set to zero.

* Add check for _refcount == 0 in Reference::Unref to avoid an underflow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants