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

Loading esm after appoptics-apm on Node 10.x results in cc error #646

Closed
onlywei opened this issue Nov 1, 2018 · 4 comments
Closed

Loading esm after appoptics-apm on Node 10.x results in cc error #646

onlywei opened this issue Nov 1, 2018 · 4 comments
Labels

Comments

@onlywei
Copy link

onlywei commented Nov 1, 2018

Hi,

I have a reproducible repository here: https://github.com/onlywei/appoptics-esm-node10
There are instructions in that repository to see the error.

I have verified that the node process indeed gets past appoptics-apm and is erroring when trying to load esm.

Also this works in Node 8.x. It only breaks in Node 10.x, which is now the LTS version of Node.

@onlywei
Copy link
Author

onlywei commented Nov 1, 2018

@bmacnaughton

@jdalton
Copy link
Member

jdalton commented Nov 1, 2018

Thanks @onlywei!

I found the issue. Working on a patch.

Update:

Related to solarwinds/appoptics-apm-node#34.

@jdalton jdalton closed this as completed Nov 1, 2018
@jdalton jdalton reopened this Nov 1, 2018
@bmacnaughton
Copy link

I can add the missing .native functions on the appoptics end. I'm suspecting that your patch might be uglier/harder than mine. If that works for you I'll get it out in the next release I do. Anyway, I've verified that it no longer causes the core dump in the simple test and that it passes our own tests but I haven't run it against any wider tests you might have in place.

I appreciate you isolating the issue in your code. Would you mind pointing me at where exactly it core dumps? I figured I'd find some C or C++ code in your repo but it's all JavaScript.

@jdalton
Copy link
Member

jdalton commented Nov 2, 2018

Hi @bmacnaughton!

No worries on testing the core dump. That was a side-effect of the missing .native method. Our code would then take a more-risky route. The fix on the esm side is to not take the more risky route if the .native method is supposed to exist but doesn't (we'll add a Node version sniff). For appoptics a simple test to ensure the wrapped fs methods have the .native is fine.

Update:

Patched 59f4ed0.

@jdalton jdalton added the bug label Nov 3, 2018
@jdalton jdalton closed this as completed Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

3 participants