search

nodejs / node-chakracore

Node.js on ChakraCore :sparkles::turtle::rocket::sparkles:
Other
1.92k stars 340 forks source link

src,test: disable `per_context.js` shim #566

Closed kfarnung closed 6 years ago

kfarnung commented 6 years ago

The Intl usage in the the shim is causing issues in ChakraCore on Linux and Mac. Since we don't necessarily need the Atomics.notify shim code either the simplest solution is to remove the shim completely for ChakraCore.

Refs: https://github.com/nodejs/node-chakracore/issues/565 Refs: https://github.com/nodejs/node-chakracore/issues/567

Checklist
kfarnung commented 6 years ago

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/279/

devsnek commented 6 years ago

just out of curiosity, what issues is it causing?

kfarnung commented 6 years ago

@devsnek We haven't completely gotten to the bottom of it yet, but during cctest something is causing the per_context initialization to run when ICU hasn't been initialized. @MSLaguana looked into it a bit and this seemed like the easiest way to work around it. Ideally we would have a way to detect the engine in this script to skip the delete of v8BreakIterator which would also unblock us.

kfarnung commented 6 years ago

Re-run: https://ci.nodejs.org/job/chakracore-test-pull-request/280/

devsnek commented 6 years ago

@kfarnung any word on this? it is a bug that chakra can't evaluate code after creating a new context right?

kfarnung commented 6 years ago

@devsnek There's no Chakra bug with evaluating code after creating a new context, it's the way that node initializes (or doesn't initialize) the engine in certain cases. In this case just accessing the Intl property without having either full ICU or small ICU data available was causing the issue for us. Since there was no good way to detect the engine so early in the pipeline, it's easier just not to use it.

Is there a plan to do more with this file? As it stands node-chakracore doesn't benefit from the current content so it was easier just to disable it.

/cc @jackhorton @MSLaguana

devsnek commented 6 years ago

@kfarnung it's being used in https://github.com/nodejs/node/pull/22835 and https://github.com/nodejs/node/pull/22844, but @jdalton raised the point that it isn't working in chakra yet.

kfarnung commented 6 years ago

Thanks for the pointers, I've updated #565 and #567 and will attempt to revert the change and see how it goes.