-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NETSCRIPT: Update ScriptDeath to extend Error #1685
Conversation
- Rename the "name" property to "hostname" so that it doesn't clash with inherited Error.name property. - Extend Error, this way if a user ends up catching it they can: - Test the type of error caught (name) and ignore it if desired - Get a stack trace and find out where they went wrong - Not require special case error printing logic for catching things that are neither strings, nor Error objects.
There's one (potential) significant issue here: ScriptDeath objects are created in a variety of circumstances, and the (small) object currently created is much cheaper than a full Is there a different way the object could be laid out that would give you more of a hint, without requiring it to be a full |
You can get a stack trace from new Error().stack but the performance impact will be from what the error does primarily, not how we're passing it. Though you said it may not be an issue, let's run everyone's batchers on dev after the merge for a quick test or do perf tests (there already are some). |
The issue is that I'm concerned there's a class of scripts I don't know about, that will be impacted. There may not be, but I vaguely feel like there might be. And there is an overhead from If it would be good enough to implement |
To help with the decision, here's a performance test of the two I whipped up. Test code is below: /** @param {NS} ns */
export async function main(ns) {
let lastError;
const tests = [1, 10, 100, 1000, 10_000, 100_000];
for (const testSize of tests) {
const startTime = performance.now();
for (let i = 0; i < testSize; i++) {
try { trace(true) } catch (error) { lastError = error; }
}
const endTime = performance.now();
ns.print(`Took ${endTime - startTime} milliseconds to throw ${testSize} NEW ScriptDeath errors.`);
}
ns.print(`ERROR: ${lastError}\n${lastError.stack}`);
for (const testSize of tests) {
const startTime = performance.now();
for (let i = 0; i < testSize; i++) {
try { trace(false) } catch (error) { lastError = error; }
}
const endTime = performance.now();
ns.print(`Took ${endTime - startTime} milliseconds to throw ${testSize} OLD ScriptDeath errors.`);
}
ns.print(`ERROR: ${lastError}\n${lastError.stack}`);
}
function trace(useNewScriptDeath) {
stack(useNewScriptDeath);
}
function stack(useNewScriptDeath) {
deep(useNewScriptDeath);
}
function deep(useNewScriptDeath) {
some(useNewScriptDeath);
}
function some(useNewScriptDeath) {
if (useNewScriptDeath)
throw new NewScriptDeath(mockProcess);
else
throw new OldScriptDeath(mockProcess);
}
const mockProcess = { 'pid': 1234, 'name': 'test.js', 'hostname': 'home' };
export class OldScriptDeath {
/** Process ID number. */
pid;
/** Filename of the script. */
filename;
/** IP Address on which the script was running */
hostname;
constructor(ws) {
this.pid = ws.pid;
this.name = ws.name;
this.hostname = ws.hostname;
Object.freeze(this);
}
}
export class NewScriptDeath extends Error {
/** Process ID number. */
pid;
/** Filename of the script. */
filename;
/** IP Address on which the script was running */
hostname;
constructor(ws) {
// Invoke the Error constructor with a meaningful message
const message = `Attempted to invoke an unsupported ns function from a killed process (${ws.name} running on ${ws.hostname} with pid ${ws.pid})`;
super(message);
// Setting the base Error.name property is important to facilitate easy
// detection, since prototype.constructor.name might be minified for them.
this.name = "ScriptDeath";
// Set own properties
this.pid = ws.pid;
this.filename = ws.name;
this.hostname = ws.hostname;
// Fix prototype, required when extending Error
Object.setPrototypeOf(this, NewScriptDeath.prototype);
Object.freeze(this);
}
} |
Could you check in the tests as well please? |
I think d0sboots's suggestion is the better approach. We can implement 2 functions:
PoC: export class ScriptDeath {
/** Process ID number. */
pid: number;
/** Filename of the script. */
name: string;
/** IP Address on which the script was running */
hostname: string;
constructor(ws: WorkerScript) {
this.pid = ws.pid;
this.name = ws.name;
this.hostname = ws.hostname;
Object.freeze(this);
}
toString(): string {
return `Attempted to invoke an NS function from a killed process (${this.name} running on ${this.hostname} with pid ${this.pid}). Call getStack() to get the stack trace.`;
}
getStack(): string | undefined {
return Error().stack;
}
} |
Won't that generate the wrong trace? |
I don't understand your question. When the player catches a |
Alpheus is right, that's the wrong stack trace - the stack where the exception was handled, not where it was thrown. Try running the example I gave in my previous description. The stack trace should at least start from "runHelper", but it will only include main. Throw an error in a function 4 or 5 deep (like in my performance test) and it'll be even more clear. |
This is what I worried about, because IIRC that's approaching the same speed of running scripts in a tight loop, currently. So in performance critical code, this very well could be an issue.
Also agreed. If the stack trace is actually needed, it must be captured at the creation of the object. What is still not clear to me is if/why the stack trace is needed, or if it's good enough to clearly identify the object in other ways. |
Is there any scenario we would be throwing on the order of 10,000 script deaths while someone is trying to run their performance-critical (e.g. H/G/W timing) loops? Only time I think this would happen is resetting / ascending, in which case hack timing loops don't matter. As you said earlier, normal scripts ending don't trigger this, only an explicit kill command trying to prematurely terminate a running script. Here's my thinking: Typically scripts are killed only one at a time. Maybe a few hundred in special circumstances (e.g. a user killing a slew of scheduled hack/grow scripts that are going against stock they want to manipulate). In these cases, its still sub-1ms, not even noticeable. The only time all thousands or tens of thousands get killed, which is still only 30ms, less than a game tick, is when resetting, at which point user-script timings don't matter anymore (they're all being killed). Even then, I would suspect that there are much slower processes being kicked off to reset most aspects of the game (mostly UI stuff), and this is just a drop in the bucket that amounts to having to wait slightly longer for |
You might have misunderstood what I was asking, but that's OK because you also answered the question just now. :) I wasn't sure if the problem was "it was unclear initially what these error objects were" or "I needed the stack traces to figure out what was going wrong in my code." From your description, it was primarily the latter. I did an audit of the code, to find all the places a new
Out of all of these, I only have high-frequency concerns about the last, and maybe the first. (But if someone is using |
Now how do we make sure that we don't accidentally introduce ScriptDeath into one of the high-speed paths in the future? Or at least signal that it's a bad idea? |
I see what you're saying. We are creating ScriptDeath objects in a few places where they aren't necessarily being thrown, and in those non-throw situations extending Error is quite frivolous... Is it worth creating a separate ScriptDeathError object (constructed with a ScriptDeath object) which we construct only as and when we're getting ready to explicitly |
The crazy+good UX path would likely entail having a config option. Ie. an interface setting in-game that controls the stack trace generation. ("Enable Stack traces on Script Death") That way you could be more aggressive on the performance for the positive case without impacting users. Also makes this PR much more trivial to merge if it's controllable. |
I'm going off on a bit of a tangent here, but I'm wondering if there are other clever ways we can invalidate the ns object that was previously passed to a script (and which is perhaps living longer than it should). Like if instead of passing an NS instance when we invoke a script's main we passed a That's probably a bigger refactor, but it seems to have been designed precisely for our use case. To be sure, we'd still need to deal with many existing edge cases such as rejecting promises-in-progress, but it may simplify others. Edit I guess upon closer inspection |
Co-authored-by: David Walker <[email protected]>
Actually in all the situations I found, we do
A combination of review diligence and general speed testing. (We do check to see how long various operations take, and if certain things, especially the common path of script creation/destruction, get slower, people with batchers will notice. The "good" answer would involve automated tracking of metrics like scripts/second, but we are nowhere near there. I've done that sort of testing professionally, and it's a pain to set up due to its inherent flakiness, and only worth it if you actually are willing to spend the time to chase down every performance regression. |
* Update ScriptDeath to extend Error - Rename the "name" property to "hostname" so that it doesn't clash with inherited Error.name property. - Extend Error, this way if a user ends up catching it they can: - Test the type of error caught (name) and ignore it if desired - Get a stack trace and find out where they went wrong - Not require special case error printing logic for catching things that are neither strings, nor Error objects. It is possible (but unlikely) that this could make killing scripts slower in some circumstances.
Summary of changes
Motivation
Not sure how many have seen one of these:
We throw a new ScriptDeath instance in the netscript
checkEnvFlags
when we want to (silently) kill code still running on a script that's been killed - whenever they next invoke an ns instance. Problem is users with their own error handling logic can catch these, and it's difficult to make heads or tails of them. If they behaved more like normal Error instances, they would be easier to deal with.As of #1513 - we no longer even display these errors in the UI when handled, so this is really just for the benefit of the user, who might still catch one of these in their try/catch handlers and not know what it is or where it came from (I was one such user).
Tests
nano test.js
This is what it looks like when you run this script twice within a few seconds on the current "stable build"
This is what it looks like when you run it on the latest "dev" build (2.6.3):
Note that the unused "errorMessage" property was recently removed - other than that, no difference.
This is what it now looks like with the change in place:
In release mode, the "internal" part of the stack trace (first 2 lines) would be minified, but the important part is that the player can now see that the line of code in test.js that caused the error was "runHelper.js:24:7":
This information, combined with the error message "Attempted to invoke an unsupported ns function from a killed process" hopefully points them in the right direction (that they were using a "bad" ns instance from the previous run)