aws/aws-xray-sdk-node

Cannot read property 'unshift' of undefined

Opened this issue · 1 comments

Error trace

TypeError: Cannot read property 'unshift' of undefined
    at Subsegment.addError (/usr/src/app/node_modules/aws-xray-sdk-core/lib/segments/attributes/subsegment.js:210:25)
    at /usr/src/app/node_modules/blah/blah/src/index.ts:305:18
    at clsBind (/usr/src/app/node_modules/cls-hooked/context.js:172:17)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

Steps to reproduce

I believe the scenario is the following:

  1. Call addError() with an error
    . triggers lines 16 and 25 below
  2. Call addError() again with the same error object
    . triggers IF on line 8 which sets this.cause without exceptions: []
  3. Call addError() again but with a different error object
    . triggers line 30 and the unshift error.

Code snippet

Subsegment.prototype.addError = function addError(err, remote) {
    if (err == null || typeof err !== 'object' && typeof (err) !== 'string') {
        throw new Error('Failed to add error:' + err + ' to subsegment "' + this.name +
            '".  Not an object or string literal.');
    }
    this.addFaultFlag();
    if (this.segment && this.segment.exception) {
        if (err === this.segment.exception.ex) {
            this.fault = true;
            this.cause = { id: this.segment.exception.cause };
            return;
        }
        delete this.segment.exception;
    }
    if (this.segment) {
        this.segment.exception = {
            ex: err,
            cause: this.id
        };
    }
    else {
        //error, cannot propagate exception if not added to segment
    }
    if (this.cause === undefined) {
        this.cause = {
            working_directory: process.cwd(),
            exceptions: []
        };
    }
    this.cause.exceptions.unshift(new CapturedException(err, remote));
};

I believe this scenario is an anti-pattern in our code but I'm wondering why line 10 in the code snippet above does not prime the array like is done further down?

i.e. this.cause = { id: this.segment.exception.cause, exceptions: [] };

Am happy to submit a PR if that won't cause any other knock on issues? This would make the code more defensive without having to put a condition check on line 30.

e.g. this.cause.exceptions && this.cause.exceptions.unshift(new CapturedException(err, remote));

Thanks

Hi @alexb-uk,

Thanks for raising this, it does seem like a missed check on our end! We would be very open to such a PR.