Don't leave isolate log debris if command execution hits error #182
Conversation
platform/v8.js
Outdated
| // Public method so it can be used in external error handlers | ||
| v8.getIsolateLog = function (workingDir, pid) { | ||
| return fs.readdirSync(workingDir).find(function (f) { | ||
| return new RegExp(`isolate-0(x)?([0-9A-Fa-f]{2,16})-${pid}-v8.log`).test(f) |
There was a problem hiding this comment.
Can you move the regexp out of the closure?
| // On error, delete any V8 isolate log left over by the failed process | ||
| fs.unlinkSync(join(args.workingDir, v8.getIsolateLog(args.workingDir, args.pid))) | ||
| throw err | ||
| }) |
There was a problem hiding this comment.
the rationale for having these files left there is to allow debugging of 0x itself. If we have access to a debug: true option, we should not remove them in this case.
There was a problem hiding this comment.
Hmm, we don't really have a general --debug equivalent in 0x, and maybe we should leave the isolate logs behind on fatal errors like this by default, so people have the log first time and don't need to replicate their error with a --debug flag.
How would you feel about having this cleanup-after-error behaviour occur on the --silent flag, then we can use that option when deliberately causing fatal errors in CI, and probably still match user expectations?
And if that flag isn't present, maybe output a message in the console telling the user where the isolate log is and that they can use a flag to not leave create logs on errors.
There was a problem hiding this comment.
I just had another look into some internals and there are two debug options: one to debug the tree and that is very different thing and second a verbose 0x output. The latter is useful for debugging 0x itself and after giving it another thought there seems to be little benefit of keeping the isolates around in an error case. In almost all cases it won't have anything to do with the specific isolate file and if it does, it should also be easily reproducible.
There was a problem hiding this comment.
So would you be happy to delete the isolate log on fatal error by default, and keep it only if the verbose flag is used?
There was a problem hiding this comment.
Yes, that would be my favorite way of doing it.
| return new RegExp(`isolate-0(x)?([0-9A-Fa-f]{2,16})-${pid}-v8.log`).test(f) | ||
| }) | ||
| const regex = new RegExp(`isolate-0(x)?([0-9A-Fa-f]{2,16})-${pid}-v8.log`) | ||
| return fs.readdirSync(workingDir).find(regex.test.bind(regex)) |
There was a problem hiding this comment.
Is the .bind() necessary at all?
There was a problem hiding this comment.
It just means we can pass the method directly without a wrapper function.
return fs.readdirSync(workingDir).find(regex.test) hits an error because regex.test looks for the regex in this.
Alternative would be like return fs.readdirSync(workingDir).find(function (f) { regex.test(f) }) referencing regex and calling its method inside a wrapper function.
Both are fine, I just don't like how redundant that wrapper function looks.
|
Latest commit switches off this behaviour if debugging is enabled as per the 0x docs, and puts a message in the console informing the user (unless --quiet or --silent are passed). For example, this hits an error, deletes the log, and gives the user a message: 0x -- node -e 'setTimeout(() => { fail() }, 1000)'
# deletes log, outputs "Fatal error in process observed by 0x. Incomplete V8 isolate log deleted. To preserve these logs, enable debugging (e.g. `DEBUG=0x* 0x my-app.js`)"
0x --quiet -- node -e 'setTimeout(() => { fail() }, 1000)'
# deletes log, outputs nothing except error details
DEBUG=0x* 0x -- node -e 'setTimeout(() => { fail() }, 1000)'
# preserves log, outputs like "Fatal error in process observed by 0x. Incomplete V8 isolate log is readable for debugging at /some/path/isolate-0x1234567890-1234-v8.log"
DEBUG=0x* 0x --quiet -- node -e 'setTimeout(() => { fail() }, 1000)'
# preserves log, outputs nothing except error details |
Depending on when the error occurs, it's possible for 0x's collect branch using the V8 platform to leave V8 isolate log files lying around in the main working directory after exit. This causes problems such as filling up CI servers of tools that use 0x and have tests for their own handlers for such errors.
To replicate, try running this from the command line. An isolate log file will be created and will persist after completion:
This PR adds a cleanup error handler that deletes any such log if there is an error in the V8 process. Run the above command in this branch while watching the working directory and you should be able to see the isolate log file be created, then deleted after a second when the error is hit.