Javascript Verticles require isolated scope: shared global scope causing unexpected behaviour

cdjones32 commented 9 years ago

Background

As JavaScript as a language is inherently single threaded (and implementations such as NodeJS and all Browser environments are all single threaded), 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 self contained 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. Reported originally under https://github.com/vert-x3/vertx-lang-js/issues/19 (the attached pull request fixes that one).

Expected behaviour

Each Verticle should have an isolated scope that can not be corrupted by other Verticles/Libraries on other threads.

Current Behaviour

All Javascript Verticles share a single Global scope. Instances of libraries (e.g. NPM modules) are shared across all deployed Verticles.

Example of Failure

The attached pull request contains two tests showing failures caused by the shared global scope - one with the 'require' method provided by JVM NPM and one a contrived leaked global variable example.

cdjones32 commented 9 years ago

Raised pull request https://github.com/vert-x3/vertx-lang-js/pull/21

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). I also split out the inlined string code while I was there, sorry... I have followed all the prerequisites on the Eclipse contribution site.

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.

cdjones32 commented 9 years ago

Examples

Require failures

An example of 'require' failing because of the shared context: https://github.com/vert-x3/vertx-lang-js/issues/19

Multiple verticles (ov the same type) with a leaked global example (in the pull request)

The example has two counters defined, one locally scoped and the other accidentally globally scoped. E.g.

var counter = 0; x = 0;

The values are incremented in a timer loop and would be expected to be the same value (and will be with a single Verticle instance). Although this is a contrived example to demonstrate, there are many examples of legitimate Javascript code that would work fine in the browser or Node but would currently fail unexpectedly with the shared context.

The result below is generated when starting multiple Verticle Instances:

Expected: 1 Actual: 1
Expected: 1 Actual: 2
Expected: 2 Actual: 3
Expected: 1 Actual: 4
Expected: 2 Actual: 5
Expected: 1 Actual: 6
Expected: 1 Actual: 7
Expected: 3 Actual: 8
Expected: 1 Actual: 9
Expected: 1 Actual: 10
Expected: 1 Actual: 11
Expected: 3 Actual: 12

java.lang.AssertionError
    at org.junit.Assert.fail(Assert.java:86)
    at org.junit.Assert.assertTrue(Assert.java:41)
    at org.junit.Assert.assertTrue(Assert.java:52)
       ...
Process finished with exit code 255

After changes: After the changes to provide isolated scopes per Verticle, the same number of instances result in:

Expected: 1 Actual: 1
Expected: 1 Actual: 1
Expected: 1 Actual: 1
Expected: 2 Actual: 2
Expected: 1 Actual: 1
Expected: 3 Actual: 3
Expected: 1 Actual: 1
Expected: 2 Actual: 2
Expected: 1 Actual: 1
Expected: 1 Actual: 1
Expected: 1 Actual: 1
Expected: 2 Actual: 2
... etc as expected

vert-x3 / vertx-lang-js