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

Exception thrown from addon #2380

Closed
gabrielschulhof opened this issue Aug 14, 2015 · 16 comments
Closed

Exception thrown from addon #2380

gabrielschulhof opened this issue Aug 14, 2015 · 16 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.

Comments

@gabrielschulhof
Copy link
Contributor

In 0.10, when throwing an exception from a C++ addon it would be printed and the process would exit. In 0.12, the exception is not printed, and the process just exits. This makes it really difficult to inform the user about missing stuff or invalid data.

@brendanashworth brendanashworth added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 14, 2015
@trevnorris
Copy link
Contributor

Will need more information, test case, etc. before we can help.

@gabrielschulhof
Copy link
Contributor Author

I'm having some difficulties producing a simple test case, because the addon where the error is happening links to a third-party library, and the exception is thrown from native code in response to an asynchronous network event received by the native library (since the main project concerns itself with providing bindings for the library).

Here's one way to reproduce the problem (tested on Fedora 21) using the actual project:

  1. Set up nvm with versions 0.12 and 0.10
  2. install unzip, make, scons, a C and C++ compiler, and headers for boost, libuuid, and libcurl
  3. wget -O iotivity-node.tar.gz https://github.com/otcshare/iotivity-node/tarball/736665c3f7c2a4440449d70020e62841e6510d8e/
  4. tar xvzf iotivity-node.tar.gz
  5. cd otcshare-iotivity-node-736665c
  6. npm install - this should download, build, and install iotivity, and build the iotivity-node bindings
  7. Turn off your firewall and open a second terminal into the directory otcshare-iotivity-node-736665c
  8. In both terminals nvm use 0.12
  9. In the first terminal, type node js/server.discoverable.js
  10. In the second terminal, type node js/client.discovery.js

At this point, the client will quit silently after a short while. Now,

  1. Interrupt the server
  2. nvm use 0.10
  3. npm install
  4. In the other terminal (where the client was running), nvm use 0.10
  5. In one terminal, node js/server.discoverable.js
  6. In the other terminal, ``node js/client.discovery.js`

At this point, after a short while you will see an exception complaining about an unsupported payload type. This is the desired behaviour, not the silence you get with node 0.12.

I'm sorry I can't come up with anything shorter :/

@bnoordhuis
Copy link
Member

When you say "exception", do you mean C++ code throwing a C++ exception or raising a JS exception with v8:::Isolate::ThrowException()?

@gabrielschulhof
Copy link
Contributor Author

The latter. The bindings code generates a JS exception that is never printed. I use nan though: https://github.com/otcshare/iotivity-node/blob/736665c3f7c2a4440449d70020e62841e6510d8e/src/structures/oc-payload.cc#L232

@gabrielschulhof
Copy link
Contributor Author

I'm still trying to stub out the library, BTW. It uses a processing loop, so, like,

setInterval( function() {
  iotivity.OCProcess();
}, 1000 );

So OCProcess(), ends up calling a function, which ends up calling a function, etc., which ultimately ends up calling js_OCPayload(), which then throws the exception.

So, I'm trying to replace the library with a simple routine that just throws an exception after so many calls to the native function.

EDIT I mean setInterval() - I updated the code above.

@gabrielschulhof
Copy link
Contributor Author

Well, it's not as simple as that, it seems. This produces the exception in the terminal with both 0.12 and 0.10.

@bnoordhuis
Copy link
Member

If you use node::MakeCallback() or nan::MakeCallback(), note that it swallows exceptions. It passes them to the domains handler if there is one but otherwise it just swallows them. There is a bug report about it but I can't find it.

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I commented out the MakeCallback() and the exception is now thrown. Thus, it seems this is indeed related to that issue.

Was nodejs/nan#284 the issue you were looking for?

@gabrielschulhof
Copy link
Contributor Author

In particular, nodejs/nan#284 (comment)

@bnoordhuis
Copy link
Member

That's probably it. I thought there was an io.js issue about it as well but maybe I've mixed up things.

@trevnorris
Copy link
Contributor

MakeCallback() currently should only be used if it's the first JS frame on the call stack. Further calls are undefined behavior. Of course this is not optimal, but the few times I've attempted to fix this in the past there have been edge cases that pop up and still make it unsafe.

@gabrielschulhof
Copy link
Contributor Author

@trevnorris how do you mean the "first JS frame on the call stack"?

The call stack in my case starts with a JS interval callback firing, which calls a binding that goes deep into the library for which I'm providing bindings. The library, in turn, calls a C function pointer I gave it earlier which gives me a pointer to the Persistent<Function> storing the JS callback I end up calling.

So, the stack is like

setInterval() callback in Javascript -> addon.OCProcess(); -> bind_OCProcess() /* C++ binding */ -> OCProcess() /* C API */ -> /* deep into C library */ -> ( *callback )( void *pointerThatIsReallyAPersistentFunction ) -> NanMakeCallback( ... ) -> /* Back into Javascript */

Does this qualify as "the first JS frame on the call stack"?

@trevnorris
Copy link
Contributor

@gabrielschulhof In your case the setInterval() callback is the first frame in the call stack. If ever you reach C++ from JS then the call back into JS shouldn't use MakeCallback().

@gabrielschulhof
Copy link
Contributor Author

@trevnorris Thanks!

@Fishrock123
Copy link
Contributor

I think this was answered? Closing.

@trevnorris
Copy link
Contributor

@Fishrock123 Um. I'll assume you meant to close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

No branches or pull requests

5 participants