-
-
Notifications
You must be signed in to change notification settings - Fork 68
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
fix(WorkerPool): trace stacks to avoid duplicated err.messages
from workers
#13
fix(WorkerPool): trace stacks to avoid duplicated err.messages
from workers
#13
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13 +/- ##
====================================
Coverage 0% 0%
====================================
Files 6 7 +1
Lines 359 371 +12
Branches 63 64 +1
====================================
- Misses 314 325 +11
- Partials 45 46 +1
Continue to review full report at Codecov.
|
2a06d3c
to
ddd6c44
Compare
ddd6c44
to
c78364e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sendilkumarn Could you post a screenshot of current output you get (for reference) ?
src/WorkerPool.js
Outdated
@@ -11,6 +11,7 @@ let workerId = 0; | |||
|
|||
class PoolWorker { | |||
constructor(options, onJobDone) { | |||
this.options = options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed or would using options
directly also be sufficient ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this is better and aligns to the style
src/WorkerPool.js
Outdated
@@ -186,7 +187,11 @@ class PoolWorker { | |||
const err = new Error(obj.message); | |||
err.message = obj.message; | |||
if (obj.stack) { | |||
err.stack = `${obj.stack}\n\tfrom thread-loader (worker ${this.id})\n${err.stack}`; | |||
let additionalStack = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err.stack = `${obj.stack}\n\nThread Loader (Worker ${this.id})`
if (this.options.stack) {
// Limit the err.stack manually in case stackTraces are to verbose
if (typeof options.stack === 'number')
Error.stackTraceLimit(options.stack)
}
err.stack += `\n\n${err.stack}`
} else {
// Only the err.message
const message = (stack) => stack.split('\n')[0]
err.stack += `\n\n${message(err.stack)}`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea. the stacktraceLimit is not a function but fairly. I will change the implementation.
options.stack
)
hmm.. this looks like a duplicate of |
Yes in terms of create-react-app (& mostly when I test it manually) (but I haven't done extensive research on various other applications) (let me know if there is any that I can check out) |
I also don't have enough knowledge about the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the output for
{Boolean}
{ stack: false )
{Number}
{ stack: 5 } // for e.g
look like now ?
Both are same. But when I increase the number to say 10 (default for stack trace) Then I see those many lines |
src/WorkerPool.js
Outdated
if (this.options.stack) { | ||
// Limit the err.stack manually in case stackTraces are to verbose | ||
if (typeof this.options.stack === 'number') { | ||
Error.stackTraceLimit = (this.options.stack); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this setting a global? It seems very non-intuitive to have a package set global configuration like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good one. But I dont think this will have a global impact. But @michael-ciniawsky would be correct one to answer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're setting it on a global so I don't see how it could be local.
From the docs (https://nodejs.org/api/errors.html#errors_error_stacktracelimit):
Changes will affect any stack trace captured after the value has been changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it changes the value in a worker thread. ( well I may be totally wrong) I will check this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the code that rethrows the error from the worker so it runs in the main thread.
I suggested another solution below.
OK, so the problem here is that The original intention was probably to preserve both stacks:
Both could be valuable depending on what exactly you're debugging (where the error actually happened vs how it "occurred" into your app). So I'd vote to keep both but cut away the duplicate message. I propose to:
Then we'll get both stacks but the message won't be duplicated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this option at all?
Do you have any reservations about my earlier suggestion?
This will be the output. If we have just removed the message from err. Since we are reconstructing the error with the same object. Mostly both of the stack remains the same. I am not convinced to add the same information again |
I don't understand why. Clearly the code frame is identical in your example. The code frame is part of the message. Therefore, if it was not "cut off", the problem is with the code that cuts it off. I'm not saying my exact algorithm is correct. But hopefully you can see that the |
Interestingly both message and stack is same in both the cases |
Doesn't it wrap and throw the same error 🤔 Ideally when you run this, they should be equal right |
Maybe worth a try const err = new Error('I\'m an Error')
const stack = err.stack.split('\n').filter((line) => line.includes('at'))
console.log(stack)
const worker = (err) => {
return new Error(err)
}
const err2 = worker(err)
const stack2 = err2.stack.split('\n').filter((line) => line.includes('at'))
console.log(stack2)
const length = stack2.length - stack.length
const result = stack2.slice(0, length).join('\n')
console.log(result) // at worker (/Users/User/path/to/index.js:6:10)
// ${obj.stack}
//
// Thread Loader (Worker 1)
//
// at worker (/Users/User/path/to/index.js:6:10) |
@michael-ciniawsky As I said in the above comment yes they are literally the same, except for the first line which says that the error is thrown from that corresponding statement's line number. Apart from that everything is same. |
@michael-ciniawsky Would you mind keeping |
const stack = (origin, worker) => {
origin = origin
.split('\n')
.filter((line) => line.includes('at'))
worker = worker
.split('\n')
.filter((line) => line.includes('at'))
.map((line) => `${line} [Thread Loader (Worker 1)]`) // needs to be replaced with `${id}`
const diff = worker.slice(0, worker.length - origin.length).join('\n')
origin.unshift(diff)
return origin.join('\n')
}
class WorkerError extends Error {
constructor (err) {
super(err)
this.name = err.name
this.message = err.message
Error.captureStackTrace(this, this.constructor)
this.stack = stack(err.stack, this.stack)
}
}
const err = new Error('I\'m an Error')
const worker = (err) => {
throw new WorkerError(err)
}
console.log(worker(err))
|
@gaearon Yep, the |
line.includes('at') is too broad. For example, throw new Error('There was a problem at MyComponent definition.'); will incorrectly match the first line (the message) as being part of the stack trace. This is why in my suggestion in #13 (comment) I search for first end of the |
I just used it |
My point is you can’t really reliably match the stack trace without knowing the message. For all we know, The only reliable way to know when the real stack trace starts is to check where the (Sorry if I misunderstood and you agree with this point. I’m stressing it because it seems important for correctness.) |
/^(\s+at\s.+\s\(.+\))/.test(line) But the diffing is already advanced anyways, replacing the stack with the updated one from the worker and maybe prepending e.g Thread Loader [Worker ${id}] // prepend
// 'Original'
${err.message}
${err.frame}
${worker.stack} // update |
@michael-ciniawsky but there are duplicates. worker.stack contains err.message in it. that causes the duplication. |
Just remove the |
I think that’s what my code in #13 (comment) was doing? |
fromErrorObj (arg) {
let obj;
if (typeof arg === 'string') {
obj = { message: arg };
} else {
obj = arg;
}
- const err = new Error(obj.message);
+ return new WorkerError(obj);
- err.message = obj.message;
- if (obj.stack) {
- const trim = (error) => {
- const { stack, message } = error;
- const trace = stack.indexOf(message) + message.length;
- return stack.substr(trace);
- };
- err.stack = `${obj.stack}\nThread Loader (Worker ${this.id})\n\n${trim(err)}`;-
- }
- err.hideStack = obj.hideStack;
- err.details = obj.details;
- return err;
} class WorkerError extends Error {
constructor (obj) {
// ...format the original {Error}
}
}
export default WorkerError
|
Ah okay I see what you mean now 🤗 @sendilkumarn Mind extracting? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sendilkumarn Thx
Can you show a screenshot how it this looks like now please :) ?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need package-lock.json
for this?
@evilebottnawi reverted |
@sendilkumarn good work! Thanks! |
Released in |
No description provided.