From ab068db9b12a9fb8619f6a94299a8d7a35d4695b Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Sun, 9 May 2010 13:54:58 -0700 Subject: [PATCH] Improve error reporting - No more single line "node.js:176:9" errors - No more strange output when error happens on first line due to module wrapper function. - A few tests to check these things --- lib/module.js | 8 +---- src/node.cc | 50 ++++++++++++++++++++--------- test/disabled/test-http-stress.js | 48 +++++++++++++-------------- test/fixtures/throws_error2.js | 3 ++ test/fixtures/throws_error3.js | 3 ++ test/simple/test-error-reporting.js | 50 +++++++++++++++++++++++++++++ 6 files changed, 115 insertions(+), 47 deletions(-) create mode 100644 test/fixtures/throws_error2.js create mode 100644 test/fixtures/throws_error3.js create mode 100644 test/simple/test-error-reporting.js diff --git a/lib/module.js b/lib/module.js index ae6c0413b6..386d796b48 100644 --- a/lib/module.js +++ b/lib/module.js @@ -376,16 +376,12 @@ Module.prototype._compile = function (content, filename) { + content + "\n});"; - try { var compiledWrapper = process.compile(wrapper, filename); var dirName = path.dirname(filename); if (filename === process.argv[1]) { process.checkBreak(); } compiledWrapper.apply(self.exports, [self.exports, require, self, filename, dirName]); - } catch (e) { - return e; - } } else { self.exports = content; } @@ -460,7 +456,5 @@ exports.runMain = function () { // Load the main module--the command line argument. process.mainModule = new Module("."); - process.mainModule.load(process.argv[1], function (err) { - if (err) throw err; - }); + process.mainModule.loadSync(process.argv[1]); } diff --git a/src/node.cc b/src/node.cc index e8bfcf8d86..d09ed2063c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -954,7 +954,7 @@ const char* ToCString(const v8::String::Utf8Value& value) { return *value ? *value : ""; } -static void ReportException(TryCatch &try_catch, bool show_line = false) { +static void ReportException(TryCatch &try_catch, bool show_line) { Handle message = try_catch.Message(); Handle error = try_catch.Exception(); @@ -975,10 +975,31 @@ static void ReportException(TryCatch &try_catch, bool show_line = false) { // Print line of source code. String::Utf8Value sourceline(message->GetSourceLine()); const char* sourceline_string = ToCString(sourceline); - fprintf(stderr, "%s\n", sourceline_string); + + // HACK HACK HACK + // + // FIXME + // + // Because of how CommonJS modules work, all scripts are wrapped with a + // "function (function (exports, __filename, ...) {" + // to provide script local variables. + // + // When reporting errors on the first line of a script, this wrapper + // function is leaked to the user. This HACK is to remove it. The length + // of the wrapper is 62. That wrapper is defined in lib/module.js + // + // If that wrapper is ever changed, then this number also has to be + // updated. Or - someone could clean this up so that the two peices + // don't need to be changed. + // + // Even better would be to get support into V8 for wrappers that + // shouldn't be reported to users. + int offset = linenum == 1 ? 62 : 0; + + fprintf(stderr, "%s\n", sourceline_string + offset); // Print wavy underline (GetUnderline is deprecated). int start = message->GetStartColumn(); - for (int i = 0; i < start; i++) { + for (int i = offset; i < start; i++) { fprintf(stderr, " "); } int end = message->GetEndColumn(); @@ -1004,13 +1025,13 @@ Local ExecuteString(Local source, Local filename) { Local script = v8::Script::Compile(source, filename); if (script.IsEmpty()) { - ReportException(try_catch); + ReportException(try_catch, true); exit(1); } Local result = script->Run(); if (result.IsEmpty()) { - ReportException(try_catch); + ReportException(try_catch, true); exit(1); } @@ -1523,7 +1544,10 @@ Handle Compile(const Arguments& args) { } Local result = script->Run(); - if (try_catch.HasCaught()) return try_catch.ReThrow(); + if (try_catch.HasCaught()) { + ReportException(try_catch, false); + exit(1); + } return scope.Close(result); } @@ -1544,7 +1568,7 @@ void FatalException(TryCatch &try_catch) { // Check if uncaught_exception_counter indicates a recursion if (uncaught_exception_counter > 0) { - ReportException(try_catch); + ReportException(try_catch, true); exit(1); } @@ -1570,7 +1594,7 @@ void FatalException(TryCatch &try_catch) { uint32_t length = listener_array->Length(); // Report and exit if process has no "uncaughtException" listener if (length == 0) { - ReportException(try_catch); + ReportException(try_catch, true); exit(1); } @@ -1899,18 +1923,14 @@ static void Load(int argc, char *argv[]) { // The node.js file returns a function 'f' -#ifndef NDEBUG TryCatch try_catch; -#endif Local f_value = ExecuteString(String::New(native_node), String::New("node.js")); -#ifndef NDEBUG if (try_catch.HasCaught()) { - ReportException(try_catch); + ReportException(try_catch, true); exit(10); } -#endif assert(f_value->IsFunction()); Local f = Local::Cast(f_value); @@ -1926,12 +1946,10 @@ static void Load(int argc, char *argv[]) { f->Call(global, 1, args); -#ifndef NDEBUG if (try_catch.HasCaught()) { - ReportException(try_catch); + ReportException(try_catch, true); exit(11); } -#endif } static void PrintHelp(); diff --git a/test/disabled/test-http-stress.js b/test/disabled/test-http-stress.js index f6e6b795d0..19a2baf99a 100644 --- a/test/disabled/test-http-stress.js +++ b/test/disabled/test-http-stress.js @@ -1,40 +1,40 @@ require("../common"); +http = require("http"); + var request_count = 1000; -var response_body = '{"ok": true}'; +var body = '{"ok": true}'; -var server = process.http.createServer(function(req, res) { - res.sendHeader(200, {'Content-Type': 'text/javascript'}); - res.sendBody(response_body); - res.finish(); +var server = http.createServer(function(req, res) { + res.writeHead(200, {'Content-Type': 'text/javascript'}); + res.write(body); + res.end(); }); -server.listen(PORT, 4024); +server.listen(PORT); var requests_ok = 0; var requests_complete = 0; -function onLoad () { +server.addListener('listening', function () { for (var i = 0; i < request_count; i++) { - process.http.cat('http://localhost:'+PORT+'/', 'utf8') - .addCallback(function (content) { - assert.equal(response_body, content) + http.cat('http://localhost:'+PORT+'/', 'utf8', function (err, content) { + requests_complete++; + if (err) { + print("-"); + } else { + assert.equal(body, content) print("."); requests_ok++; - requests_complete++; - if (requests_ok == request_count) { - puts("\nrequests ok: " + requests_ok); - server.close(); - } - }) - .addErrback(function() { - print("-"); - requests_complete++; - //process.debug("error " + i); - }); + } + if (requests_complete == request_count) { + puts("\nrequests ok: " + requests_ok); + server.close(); + } + }); } -} +}); process.addListener("exit", function () { - assert.equal(request_count, requests_complete); - assert.equal(request_count, requests_ok); + assert.equal(request_count, requests_complete); + assert.equal(request_count, requests_ok); }); diff --git a/test/fixtures/throws_error2.js b/test/fixtures/throws_error2.js new file mode 100644 index 0000000000..6185815341 --- /dev/null +++ b/test/fixtures/throws_error2.js @@ -0,0 +1,3 @@ + +JSON.parse(undefined); + diff --git a/test/fixtures/throws_error3.js b/test/fixtures/throws_error3.js new file mode 100644 index 0000000000..aaf774b2ef --- /dev/null +++ b/test/fixtures/throws_error3.js @@ -0,0 +1,3 @@ +process.nextTick(function () { + JSON.parse(undefined); +}); diff --git a/test/simple/test-error-reporting.js b/test/simple/test-error-reporting.js new file mode 100644 index 0000000000..aa02de92a9 --- /dev/null +++ b/test/simple/test-error-reporting.js @@ -0,0 +1,50 @@ +require("../common"); +exec = require('child_process').exec, +path = require('path'); + +exits = 0; + +function errExec (script, callback) { + var cmd = process.argv[0] + ' ' + path.join(fixturesDir, script); + return exec(cmd, function (err, stdout, stderr) { + // There was some error + assert.ok(err); + + // More than one line of error output. + assert.ok(stderr.split('\n').length > 2); + + // Assert the script is mentioned in error output. + assert.ok(stderr.indexOf(script) >= 0); + + // Proxy the args for more tests. + callback(err, stdout, stderr); + + // Count the tests + exits++; + + puts('.'); + }); +} + + +// Simple throw error +errExec('throws_error.js', function (err, stdout, stderr) { + assert.ok(/blah/.test(stderr)); +}); + + +// Trying to JSON.parse(undefined) +errExec('throws_error2.js', function (err, stdout, stderr) { + assert.ok(/JSON/.test(stderr)); +}); + + +// Trying to JSON.parse(undefined) in nextTick +errExec('throws_error3.js', function (err, stdout, stderr) { + assert.ok(/JSON/.test(stderr)); +}); + + +process.addListener('exit', function () { + assert.equal(3, exits); +});