requirejs / r.js

Runs RequireJS in Node and Rhino, and used to run the RequireJS optimizer
Other
2.57k stars 673 forks source link

Java process does not exit when r.js is run with Rhino with config file and optimize="closure" #566

Open akatkere opened 11 years ago

akatkere commented 11 years ago

Simple test case:

build.js: ({ appDir: "./src/", baseUrl: "./", dir: "./build/", modules: [ { name: "test", } ], removeCombined: true, optimize: "closure", })

src/test.js: { }

Run r.js with:

java -classpath ../lib/js.jar:../lib/compiler.jar org.mozilla.javascript.tools.shell.Main ../lib/r.js -o build.js

Tracing dependencies for: test Minifying file: /Users/akatkere/tmp/rjs-poc/test/build/test.js

test.js

test.js

[ does not exit]

Thread dump shows a couple of closure compiler threads that are not doing anything. Even if I optimize one module explicitly via command line, it still hangs:

java -classpath ../lib/js.jar:../lib/compiler.jar org.mozilla.javascript.tools.shell.Main ../lib/r.js -o baseUrl=./src/ name=test out=build/test-built.js optimize=closure

Tracing dependencies for: test Minifying file: /Users/akatkere/tmp/rjs-poc/test/build/test-built.js

/Users/akatkere/tmp/rjs-poc/test/build/test-built.js

/Users/akatkere/tmp/rjs-poc/test/src/test.js

[ does not exit]

I was able to get past the issue by explicitly passing a quit function to build:

diff --git a/dist/r.js b/dist/r.js
index 309ceaf..95ec849 100644
--- a/dist/r.js
+++ b/dist/r.js
@@ -24115,7 +24115,7 @@ define('build', function (require) {
      * This function does not return a status, it should throw an error if
      * there is a problem completing the build.
      */
-    build = function (args) {
+    build = function (args, quit) {
         var buildFile, cmdConfig, errorMsg, errorStack, stackMatch, errorTree,
             i, j, errorMod,
             stackRegExp = /( {4}at[^\n]+)\n/,
@@ -24145,7 +24145,7 @@ define('build', function (require) {
             }

             return build._run(cmdConfig);
-        }).then(null, function (e) {
+        }).then(function() { quit(0); }, function (e) {
             var err;

             errorMsg = e.toString();
@@ -25920,7 +25920,7 @@ define('build', function (require) {
                     quit(1);
                 };

-                build(config).then(done, done).then(callback, errback);
+                build(config, quit).then(done, done).then(callback, errback);
             };

             requirejs({
@@ -26006,7 +26006,7 @@ require({
     }
 },       ['env!env/args', 'env!env/quit', 'logger', 'build'],
 function (args, quit, logger, build) {
-    build(args).then(function () {}, function (err) {
+    build(args, quit).then(function () {}, function (err) {
         logger.error(err);
         quit(1);
     });
jrburke commented 10 years ago

I am not sure what is going on here, as you note, it only happens if closure compiler is used. If I use uglifyjs or set optimize: 'none', it is fine. With closure, it does not hang indefinitely, it eventually quits for me, but it does take a long time for it to happen. So I expect it to be an issue with closure compiler in some way.

Unfortunately I cannot use that diff as-is, as that function is designed to be called by code that may want to continue past that point, and not just quit.

So I am not sure how best to go about making this better. Unfortunately I do not have the java skills any more to dig into what is going under the covers with closure compiler. Maybe the interface changed and if I were to call it differently it would work better. I will be looking to someone in the community with Java experience to dig into the root cause.

As a data point for anyone looking into this: I tried on Mac OS X with the closure compiler in the r.js repo, with java -version of:

java version "1.6.0_65"
Java(TM) SE Runtime Environment (build 1.6.0_65-b14-462-11M4609)
Java HotSpot(TM) 64-Bit Server VM (build 20.65-b04-462, mixed mode)
akatkere commented 10 years ago

@jrburke, isn't there any other place where we know we are done optimizing where quit can be called? As far as I can see from the thread dump, closure compiler has a couple of non daemon threads running, and the process will not exit until they exit. I was looking into filing a defect against closure compiler, but the difficulty there is calling Java API is not a documented way of using closure compiler (as far as I can see). So a workaround on r.js since would be much simpler than trying to get the closure compiler side "fixed".

jrburke commented 10 years ago

The problem is that other code, like embedded use inside of other .js programs, requires that the code not exit at that point -- the build() API is just an API to do the build, other code decides when to quit.

It is also disconcerting that this is a specific issue with a specific minifier: uglifyjs and not minifying does not cause a hang. I am unsure of what fix would help, but quitting at the point in the diff above is not an option -- it would break existing code and uses of r.js.