malaporte / nashorn-commonjs-modules

CommonJS modules support for Nashorn
MIT License
108 stars 31 forks source link

Only clear ref-cache for current module being resolved #20

Closed kipz closed 7 years ago

kipz commented 7 years ago

With out this, the new test case itCanShortCircuitDeepCircularRequireReferences causes a StackOverflowError because we cleared the whole ref-cache when we unwind the stack, rather than just removing the resolved module from it.

kipz commented 7 years ago

Yeah I removed this and updated the PR.

Thanks.

On 5 May 2017, at 08:42, Martin Laporte notifications@github.com wrote:

@malaporte commented on this pull request.

In src/main/java/com/coveo/nashorn_modules/Module.java https://github.com/coveo/nashorn-commonjs-modules/pull/20#discussion_r114944680:

@@ -131,7 +132,11 @@ public Object require(String module) throws ScriptException {

 } finally {
   // Finally, remove successful resolves from the refCache
  • refCache.remove();
  • if(requestedFullPath != null){
  • refCache.get().remove(requestedFullPath);
  • }else{
  • refCache.remove();; Since you're essentially cleaning up in finally I can't really see why you'd flush the entire cache at this point, vs just letting other stuff (owned by code higher up in the call stack). But I haven't thought about this for hours either. I'd say that if you can come up with a scenario where this is needed (with a test to prove it 😁) it's fine, otherwise it might not be needed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/coveo/nashorn-commonjs-modules/pull/20#discussion_r114944680, or mute the thread https://github.com/notifications/unsubscribe-auth/ABx1QkrVGpynxH-z3gyph648S9ow2tpCks5r2tLTgaJpZM4NLctd.