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

Polluted __stackCleaned__ property #503

Closed
moll opened this Issue Feb 19, 2015 · 19 comments

Comments

Projects
None yet
4 participants
@moll

moll commented Feb 19, 2015

Hey,

I'm not sure what __stackCleaned__ does, but it's set as an enumerable property for errors that flow through Bluebird. Please set it as a non-enumerable property as it's otherwise changing the public contract for all error objects.

Thanks!

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 19, 2015

Collaborator

@moll bluebird only does this in debug mode for stitching long stack traces.

@petkaantonov when you're back from the coke, can this move to CapturedTrace so it won't be exposed to the outside in these cases?

Collaborator

benjamingr commented Feb 19, 2015

@moll bluebird only does this in debug mode for stitching long stack traces.

@petkaantonov when you're back from the coke, can this move to CapturedTrace so it won't be exposed to the outside in these cases?

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Feb 19, 2015

Sorry, I'm not a user of Bluebird myself so I don't know exactly how Bluebird ends up in debug mode when used by Knex. All I see is the __stackCleaned__ enumerable property on errors.

moll commented Feb 19, 2015

Sorry, I'm not a user of Bluebird myself so I don't know exactly how Bluebird ends up in debug mode when used by Knex. All I see is the __stackCleaned__ enumerable property on errors.

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 19, 2015

Collaborator

@moll you'll have to ask @tgriesser that. Note that debug mode is very useful.

I agree that bluebird should not just do that - that = true can be made into a es5.defineProperty or something similar. Still not sure why you're enumerating properties off errors but ok.

I'd make a PR but Petka will likely write this faster than go through my code and merge it anyway :D

Collaborator

benjamingr commented Feb 19, 2015

@moll you'll have to ask @tgriesser that. Note that debug mode is very useful.

I agree that bluebird should not just do that - that = true can be made into a es5.defineProperty or something similar. Still not sure why you're enumerating properties off errors but ok.

I'd make a PR but Petka will likely write this faster than go through my code and merge it anyway :D

@benjamingr benjamingr closed this Feb 19, 2015

@benjamingr benjamingr reopened this Feb 19, 2015

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Feb 19, 2015

Collaborator

Whoops, that was butterfingers

Collaborator

benjamingr commented Feb 19, 2015

Whoops, that was butterfingers

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

I agree that bluebird should not just do that - that = true can be made into a es5.defineProperty or something similar. Still not sure why you're enumerating properties off errors but ok.

It's necessary e.g. when marshaling an error from another process but in this case the property is desirable to stay enumerated so that the main process doesn't reprocess the stack property so making it non-enumerable would actually break the only use case I can think for enumerating errors!

Owner

petkaantonov commented Feb 19, 2015

I agree that bluebird should not just do that - that = true can be made into a es5.defineProperty or something similar. Still not sure why you're enumerating properties off errors but ok.

It's necessary e.g. when marshaling an error from another process but in this case the property is desirable to stay enumerated so that the main process doesn't reprocess the stack property so making it non-enumerable would actually break the only use case I can think for enumerating errors!

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

@moll I am not convinced this has any effect on the public contract of an error, much like jQuery adding jQuery9103401394031 property to all dom elements doesn't have any effect on the public api of the DOM. If there is some real practical problem with this, please describe it - as making it non-enumerable creates real practical problems as noted in the comment above.

Owner

petkaantonov commented Feb 19, 2015

@moll I am not convinced this has any effect on the public contract of an error, much like jQuery adding jQuery9103401394031 property to all dom elements doesn't have any effect on the public api of the DOM. If there is some real practical problem with this, please describe it - as making it non-enumerable creates real practical problems as noted in the comment above.

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Feb 19, 2015

It's necessary e.g. when marshaling an error from another process but in this case the property is desirable to stay enumerated so that the main process doesn't reprocess the stack property so making it non-enumerable would actually break the only use case I can think for enumerating errors!

Umm, I don't get the bit about "main process doesn't reprocess". If there's code transforming errors, then all bets are off anyhow, wouldn't you say? Not to mention that property only works if there's Bluebird on the other end. I'm using native promises usually.

Well, as said above, I'm sending errors out on the wire and seeing weird _stackCleaned__ properties that I didn't add to errors. Don't want them going to remote clients, to HTTP responses etc. ;-)

That's besides the point, but toSerializableError is doing it wrong, too. It's not iterating over inherited properties. ^_^

moll commented Feb 19, 2015

It's necessary e.g. when marshaling an error from another process but in this case the property is desirable to stay enumerated so that the main process doesn't reprocess the stack property so making it non-enumerable would actually break the only use case I can think for enumerating errors!

Umm, I don't get the bit about "main process doesn't reprocess". If there's code transforming errors, then all bets are off anyhow, wouldn't you say? Not to mention that property only works if there's Bluebird on the other end. I'm using native promises usually.

Well, as said above, I'm sending errors out on the wire and seeing weird _stackCleaned__ properties that I didn't add to errors. Don't want them going to remote clients, to HTTP responses etc. ;-)

That's besides the point, but toSerializableError is doing it wrong, too. It's not iterating over inherited properties. ^_^

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

Processing the stack means removing library internal callers from it and adding the stacks from previous events. If it's reprocessed that means you will get unreadable stack. So if the client/sub-process has already processsed the stack and removes the property when transferring it to server/main-process, the stack will be unreadable because the server/main-process will reprocess the stack.

That's besides the point, but toSerializableError is doing it wrong, too. It's not iterating over inherited properties. ^_^

I don't care about inherited properties except .name which is copied explicitly.

Owner

petkaantonov commented Feb 19, 2015

Processing the stack means removing library internal callers from it and adding the stacks from previous events. If it's reprocessed that means you will get unreadable stack. So if the client/sub-process has already processsed the stack and removes the property when transferring it to server/main-process, the stack will be unreadable because the server/main-process will reprocess the stack.

That's besides the point, but toSerializableError is doing it wrong, too. It's not iterating over inherited properties. ^_^

I don't care about inherited properties except .name which is copied explicitly.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

Also how exactly are you sending errors? If you send them as is, you don't get anything:

JSON.stringify(new Error("error"))
"{}"
Owner

petkaantonov commented Feb 19, 2015

Also how exactly are you sending errors? If you send them as is, you don't get anything:

JSON.stringify(new Error("error"))
"{}"
@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

Using:

Promise.delay(1).then(require("fs").readFile);

Unprocessed stack:

Unhandled rejection TypeError: path must be a string
    at TypeError (native)
    at fs.readFile (fs.js:242:11)
    at tryCatcher (/home/petka/bluebird/js/release/util.js:9:31)
    at Promise._settlePromiseFromHandler (/home/petka/bluebird/js/release/promise.js:473:31)
    at Promise._settlePromiseAt (/home/petka/bluebird/js/release/promise.js:571:18)
    at Promise._settlePromises (/home/petka/bluebird/js/release/promise.js:678:14)
    at Async._drainQueue (/home/petka/bluebird/js/release/async.js:75:16)
    at Async._drainQueues (/home/petka/bluebird/js/release/async.js:85:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/home/petka/bluebird/js/release/async.js:13:14)
    at processImmediate [as _immediateCallback] (timers.js:321:17)

Note how the stack is literally useless, there is not a single line that refers to anywhere in my code.

Processed stack:

Unhandled rejection TypeError: path must be a string
    at TypeError (native)
    at fs.readFile (fs.js:242:11)
    at processImmediate [as _immediateCallback] (timers.js:321:17)
From previous event:
    at Object.<anonymous> (/home/petka/bluebird/throwaway.js:4:18)
    at Module._compile (module.js:446:26)
    at Object.Module._extensions..js (module.js:464:10)
    at Module.load (module.js:341:32)
    at Function.Module._load (module.js:296:12)
    at Function.Module.runMain (module.js:487:10)
    at startup (node.js:111:16)
    at node.js:799:3
Owner

petkaantonov commented Feb 19, 2015

Using:

Promise.delay(1).then(require("fs").readFile);

Unprocessed stack:

Unhandled rejection TypeError: path must be a string
    at TypeError (native)
    at fs.readFile (fs.js:242:11)
    at tryCatcher (/home/petka/bluebird/js/release/util.js:9:31)
    at Promise._settlePromiseFromHandler (/home/petka/bluebird/js/release/promise.js:473:31)
    at Promise._settlePromiseAt (/home/petka/bluebird/js/release/promise.js:571:18)
    at Promise._settlePromises (/home/petka/bluebird/js/release/promise.js:678:14)
    at Async._drainQueue (/home/petka/bluebird/js/release/async.js:75:16)
    at Async._drainQueues (/home/petka/bluebird/js/release/async.js:85:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/home/petka/bluebird/js/release/async.js:13:14)
    at processImmediate [as _immediateCallback] (timers.js:321:17)

Note how the stack is literally useless, there is not a single line that refers to anywhere in my code.

Processed stack:

Unhandled rejection TypeError: path must be a string
    at TypeError (native)
    at fs.readFile (fs.js:242:11)
    at processImmediate [as _immediateCallback] (timers.js:321:17)
From previous event:
    at Object.<anonymous> (/home/petka/bluebird/throwaway.js:4:18)
    at Module._compile (module.js:446:26)
    at Object.Module._extensions..js (module.js:464:10)
    at Module.load (module.js:341:32)
    at Function.Module._load (module.js:296:12)
    at Function.Module.runMain (module.js:487:10)
    at startup (node.js:111:16)
    at node.js:799:3
@bendrucker

This comment has been minimized.

Show comment
Hide comment
@bendrucker

bendrucker Feb 19, 2015

Knex is not turning on debug mode except within its own test suite

bendrucker commented Feb 19, 2015

Knex is not turning on debug mode except within its own test suite

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

btw it's not about debug mode but long stack traces which are enabled by various things:

  • NODE_ENV=development environment variable
  • BLUEBIRD_DEBUG=anything environment variable
  • Calling Promise.longStackTraces()
Owner

petkaantonov commented Feb 19, 2015

btw it's not about debug mode but long stack traces which are enabled by various things:

  • NODE_ENV=development environment variable
  • BLUEBIRD_DEBUG=anything environment variable
  • Calling Promise.longStackTraces()
@bendrucker

This comment has been minimized.

Show comment
Hide comment
@bendrucker

bendrucker Feb 19, 2015

Yup, I know. We use long stack traces in the test suite only so I'm not sure what @moll is seeing. If you can narrow it down to Knex open an issue.

bendrucker commented Feb 19, 2015

Yup, I know. We use long stack traces in the test suite only so I'm not sure what @moll is seeing. If you can narrow it down to Knex open an issue.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

Already made non-enumerable in 2.9.11

Owner

petkaantonov commented Feb 19, 2015

Already made non-enumerable in 2.9.11

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Feb 19, 2015

Perfect. Thanks, @petkaantonov!
@bendrucker: It could've just been the NODE_ENV that enabled it.

Also how exactly are you sending errors?

JSON stringified, yep, but as the property was enumerable and on the error itself, it got serialized.

moll commented Feb 19, 2015

Perfect. Thanks, @petkaantonov!
@bendrucker: It could've just been the NODE_ENV that enabled it.

Also how exactly are you sending errors?

JSON stringified, yep, but as the property was enumerable and on the error itself, it got serialized.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Feb 19, 2015

Owner

I'm just curious if you are using json.stringify directly on errors, the result is not useful so why do it? :P

Owner

petkaantonov commented Feb 19, 2015

I'm just curious if you are using json.stringify directly on errors, the result is not useful so why do it? :P

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Feb 19, 2015

Usually I use it together with StandardError.js to get the name and message as enumerable too. And then any extra properties the errors might have.

That strategy coupled with something like PgError.js ends up being immensely useful over the regular duck typed Error instances.

moll commented Feb 19, 2015

Usually I use it together with StandardError.js to get the name and message as enumerable too. And then any extra properties the errors might have.

That strategy coupled with something like PgError.js ends up being immensely useful over the regular duck typed Error instances.

@moll

This comment has been minimized.

Show comment
Hide comment
@moll

moll Feb 19, 2015

Subclassing errors happens to be the reason I find ignoring inherited properties, frankly, retarded. Rather than mashing everything repeatedly on instances, setting them up in the prototype is a lot more elegant. That does require a custom toJSON however. JSON.stringify is equally retarded in this regard. ^_^

moll commented Feb 19, 2015

Subclassing errors happens to be the reason I find ignoring inherited properties, frankly, retarded. Rather than mashing everything repeatedly on instances, setting them up in the prototype is a lot more elegant. That does require a custom toJSON however. JSON.stringify is equally retarded in this regard. ^_^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment