Skip to content

Question, why ecma_create_error_reference are needed, as error are also an object. #4551

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

Open
lygstate opened this issue Jan 29, 2021 · 5 comments

Comments

@lygstate
Copy link
Contributor

When I am using iot.js NAPI, the NAPI treat error as object, but in jerryscript,
error is not an object but a error reference to object. That's inconsistence with n-api and
getting iot.js napi doesn't support for external errors.

@rerobika
Copy link
Member

rerobika commented Feb 4, 2021

The simple reason is that the current API's error handling differs from the internal. Internally only 1 pedinding expection is allowed. These execptions are transformed into error_references, which "saves" the thrown error. Therefore you can hold several previous exceptions in the C stack wrapped into error references. However, there are plans to change it for the next release, so stay focused.

@lygstate
Copy link
Contributor Author

lygstate commented Feb 4, 2021

The simple reason is that the current API's error handling differs from the internal. Internally only 1 pedinding expection is allowed. These execptions are transformed into error_references, which "saves" the thrown error. Therefore you can hold several previous exceptions in the C stack wrapped into error references. However, there are plans to change it for the next release, so stay focused.

I faced some sorts of memory leak problems when migrating sqlite napi to iotjs. and found once we raise a exception in javascript
and through to the C caller(this caller is an external native envet(file system io)), and the error would cause memory in JERRY_SYSTEM_MEMORY=ON options(JERRY_SYSTEM_MEMORY=OFF won't raise memory leak). The memory leak detected by multiple tools( valgrind and drmemory), I have no idea how to fixes, because I don't know which object are leaked.

@rerobika
Copy link
Member

rerobika commented Feb 4, 2021

Just a side note, NAPI is not part of JerryScript, it belongs to IoT.js. Beside that's not the right place to discuss I don't really understand what kind of help/answers do you expect. You faced with a problem, okay but whats the testcase, build environtment, expected behaviour, how to reproduce it? However, if you really meant to report it to JerryScript, could you show a reduced expamle of this problem which only uses Jerry API (without IoT.js and NAPI) and reproduces the same memory leak?

@lygstate
Copy link
Contributor Author

lygstate commented Feb 6, 2021

Just a side note, NAPI is not part of JerryScript, it belongs to IoT.js. Beside that's not the right place to discuss I don't really understand what kind of help/answers do you expect. You faced with a problem, okay but whats the testcase, build environtment, expected behaviour, how to reproduce it? However, if you really meant to report it to JerryScript, could you show a reduced expamle of this problem which only uses Jerry API (without IoT.js and NAPI) and reproduces the same memory leak?

Hi, thanks for you help, I spend a week to tracing this memory leak problem, and now it's figured out that's a iot.js napi wrapping bug. but also I am facing that the error object are hard to debugging with.
As in Javascript, error can be any value of
undefined/boolean/number/object/string and so on, but in C world, that's another things, at least the node_api wanna setting property on error object. I create a issue here mainly not about tracking the memory leak problem, but what's the design improvement about the Error object. As you said However, there are plans to change it for the next release, so stay focused.
What's the plan you talking about?

@lygstate
Copy link
Contributor Author

lygstate commented Dec 7, 2021

Does #4829 fixed this?

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

2 participants