Closed cdjones32 closed 9 years ago
purplefox
commented
9 years ago Hi Chris,
If you could separate the reproducers from your solution, that would help.
Before looking at any possible solutions, I'd like to reproduce the issue and do some investigations :)
cdjones32
commented
9 years ago Hi Tim,
Thank you for the response. I have created a separate PR which includes only the tests as requested.
https://github.com/vert-x3/vertx-lang-js/pull/22
I have recommended VertX3 for a couple of big pilot projects for a customer where a lot of the implementation will be in Javascript (client requirement), so I am very keen to help out on anything I can (not just on the lang-js side).
Regards Chris
Tim Fox mailto:notifications@github.com 26 May 2015 10:12 pm
Hi Chris,
If you could separate the reproducers from your solution, that would help.
Before looking at any possible solutions, I'd like to reproduce the issue and do some investigations :)
— Reply to this email directly or view it on GitHub https://github.com/vert-x3/vertx-lang-js/pull/21#issuecomment-105502196.
purplefox
commented
9 years ago Chris,
Thanks for doing this. Normally I wouldn't ask and it seems a little lazy on our behalf, it's just that we have a large workload for the up-coming 3.0 release so every little bit that makes things simpler for us to process, helps :)
cdjones32
commented
9 years ago Hi Tim,
Not a problem at all. I know the dates you are pushing for, so I can understand.
I'm actually going on leave from today for a week, so I'll probably be a bit slow to respond (i.e. when the wife isn't looking). Bad timing I know, but that's just how it worked out...
Thanks again for taking the time to look into it.
Regards Chris
Tim Fox mailto:notifications@github.com 27 May 2015 5:27 pm
Chris,
Thanks for doing this. Normally I wouldn't ask and it seems a little lazy on our behalf, it's just that we have a large workload for the up-coming 3.0 release so every little bit that makes things simpler for us to process, helps :)
— Reply to this email directly or view it on GitHub https://github.com/vert-x3/vertx-lang-js/pull/21#issuecomment-105796999.
As JavaScript as a language is inherently single threaded (and implementations such as NodeJS and all Browser environments are as well), the majority of Javascript Libraries are not designed to be accessed in a multi threaded environment. Having a single global context for all Javascript Verticles (and Verticle instances) and their dependencies (of which VertX states compatibility with NPM) is extremely likely to introduce race conditions on either access to global variables or even instance variables within a script that is not designed to provide atomic updates (e.g. for internal state).
The JVM NPM library itself (which is used by VertX for NPM/CommonJS support) contains a serious race condition for its internal require cache, where multiple concurrent access (e.g. vert run service.js -instances 10) causes the return value from the 'require' call to be undefined for an indeterminate number of the instances. This patch fixes that issue among potentially many others.
The main changes are to JSVerticleFactory which is really minor refactoring and the introduction of per Verticle ScriptContexts so that there is a 'global' Script Context per standard Verticle Thread (which is the expected behaviour and consistent with the standard VertX approach).
Also added tests related to two manifestations of the issue which fail before this change and pass afterward. All existing tests execute without issue.
Again, I believe that this behaviour is what pretty much all Javascript developers would be expecting. If they do need access to shared data, that is what the VertX API and Clustering is there to provide. The javascript environment of each thread should be isolated from the other threads.