Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

src,test: disable per_context.js shim #566

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Jul 10, 2018

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: #565
Refs: #567

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@kfarnung kfarnung self-assigned this Jul 10, 2018
@kfarnung kfarnung requested a review from MSLaguana July 10, 2018 00:24
@kfarnung
Copy link
Contributor Author

@devsnek
Copy link
Member

devsnek commented Jul 10, 2018

just out of curiosity, what issues is it causing?

@kfarnung
Copy link
Contributor Author

@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
Copy link
Contributor Author

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: nodejs#565
Refs: nodejs#567
PR-URL: nodejs#566
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@kfarnung kfarnung merged commit 4236937 into nodejs:master Jul 10, 2018
@kfarnung kfarnung deleted the percontext branch July 10, 2018 20:51
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jul 10, 2018
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: nodejs#565
Refs: nodejs#567
PR-URL: nodejs#566
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>
@jdalton jdalton mentioned this pull request Sep 14, 2018
4 tasks
@devsnek
Copy link
Member

devsnek commented Sep 14, 2018

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

@kfarnung
Copy link
Contributor Author

@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
Copy link
Member

devsnek commented Sep 14, 2018

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

@kfarnung
Copy link
Contributor Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants