Multiple concurrent calls to 'require' failing

[cdjones32]

Multiple concurrent calls to the 'require' method (e.g. when deploying a JS Verticle which uses require with the '-instances x' argument) result in some of the instances receiving an empty object instead of the expected dependency.

I have started looking into the issue and I've written a test that replicates it here: edit removed this branch there is a new one with the fixes related to #20

Result of the above test:

test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: ok
test_multiple_concurrent_requires: ok
test_multiple_concurrent_requires: ok
test_multiple_concurrent_requires: ok
test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: failed: Property is null
test_multiple_concurrent_requires: 6

java.lang.AssertionError: Failure count must be 0

--- End of issue specific text ---

I haven't submitted it as a pull request as I will attempt to implement a fix on the same branch; however, this being my first public commit I wanted to ensure I was doing everything properly before submitting. I know my naming convention is probably too long, I've put an integration test in the require test where a simpler one would probably suffice, I've left log statements in (just to more easily show the issue) etc so I apologise for that in advance. If I should be doing anything at all differently please feel free to critique it and I will sort it out.

I also wasn't sure whether I should be posting this here or on the Eclipse Bugzilla, so apologies if this is incorrect.

BTW: This is excellent work... I am very excited about the VertX 3 release and the Nashorn integration provided by this module.

[cdjones32]

Update: This comment is outdated and no longer relevant. See below

An update - I found that the issue was being caused by jvm-npm.js. The code that was adding files to the cache was adding empty objects to the cache when it was initially setting the exports object.

I have added a check for the value being either null or an empty object '{}' and prevent those values from going into the cache.

If the fix and the test are acceptable, I will clean up the test (remove the logs etc) and submit the pull request. Let me know if there is anything else that would help or you would like me to do.

[cdjones32]

Update: This comment is outdated and no longer relevant. See below

Sorry... I just realised that the fix I outlined above introduces a Stack Overflow for modules with circular dependencies. One example is vertx-apex-js/route.js and vertx-apex-js/routing_context.js which have circular references to each other.

This will need some more thought as the jvm-npm is obviously wrong, however I can't currently think how it could be fixed easily in its current form. The recursive dependencies don't seem correct either, but I don't want to introduce an error into other VertX modules.

[cdjones32]

I've gone back over the issue, and I no longer think that the issue is necessarily jvm-npm - but that jvm-npm is expecting to be executed in a single threaded environment. The issue is occurring because JSVerticleFactory is instantiating multiple threads (one for each Vertical instance); however, all Javascript Verticle threads are sharing a single global scope and a single Nashorn Engine.

The issue raised above is occurring because multiple threads are updating what is essentially global code that was designed to be single threaded (i.e. the Require.cache in jvm-npm). I would expect that many npm libraries make similar assumptions as neither node nor browser environments support multi-threaded updates.

I am now looking for guidance on how to proceed…

• Fixing the isolation of the Javascript Verticles would probably be the simplest fix (I’ve already tested it) and IMHO would be the expected behaviour. This will come at a cost of higher memory usage per deployed Vertical.
• If not, the jvm-npm will need to be modified to allow multi-threaded updates. This will not be easy in its current form and it may actually be simpler to replace it.

I will continue to look at implementing the Javascript isolation (I have already implemented it to test, but I want to see if I can get the memory usage down a bit); however, any comments/thoughts would be appreciated.

[purplefox]

Thanks. We had a similar issue in Ruby with concurrent requires....

I think what we need to do here is synchronize access to the require() method so it's never executed concurrently

[cdjones32]

Hi Tim, Thanks for the response and apologies for the delay in reply.

I have actually opened another issue that represents what I think is the core issue. The associated pull request fixes this issue as well. There is a lot of reasoning in that issue so I'll leave it short here.

The other issue is #20
The pull request is #21

** update **
Added a PR for the reproducers only: #22