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

promise: warning on unhandled rejection #8217

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@jasnell
Member

jasnell commented Aug 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

promises

Description of change

Emit warnings on unhandled rejection. This is a completion of @benjamingr's PR here that's been sitting dormant for a bit.

/cc @Fishrock123 @trevnorris

@jasnell

This comment has been minimized.

@chrisdickinson

View changes

lib/internal/process/promises.js Outdated
// TODO(petkaantonov) Take some default action, see #830
process.emitWarning('Possibly unhandled promise rejection ' +
`(rejection id: ${uid}): ${reason}`,
'UnhandledPromiseRejectionWarning');

This comment has been minimized.

@chrisdickinson

chrisdickinson Aug 22, 2016

Contributor

My quibble from the last discussion holds here — at the time of logging it is definitely unhandled.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 22, 2016

Member

I still agree.

@chrisdickinson

This comment has been minimized.

Contributor

chrisdickinson commented Aug 22, 2016

It's been a bit since I've been in the promises-in-core headspace, but, IIRC, I believe where I settled after the discussion last spring was thus:

  1. We should definitely warn on unhandled rejection in the absence of other listeners by default for at least one major version.
  2. We should definitely fix the interaction between Promises and AsyncWrap/Domains.
  3. We should consider crashing on unhandled rejection in the absence of listeners in a subsequent major.

This PR addresses point (1) and, my nits around the wording of the error aside, LGTM. Point (2) is tangential to this conversation, except for release timing relating to point (3). Point (3) relates to this conversation in that depending on the nature of the warning we log, we could introduce the breaking behavior of unhandled rejection sooner — that is, if we log a deprecation warning in v6, we could introduce crash-on-unhandled-rejection in v7.

My question is: do we want to — or need to — make a decision on that in advance of landing this PR? I note that with this change, we're taking a subset of programs that would have previously been "quiet" and making them noisy. This implicitly disincentivizes users from designing applications and packages that rely on handling previously unhandled rejections. The work necessary to correct the new logging behavior is the same as the work necessary to correct the proposed crashing behavior. My sense is that folks who object to crashing on unhandled rejection may also object to warning on unhandled rejection, but I would like to check that assumption.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 22, 2016

Adding this as a deprecation warning is a semver-major, which means the
warning would not go into a release until v7. The earliest we could throw
is v8.

Adding this as a regular warning now, then changing it to a deprecation
warning later may cause some confusion for those handling the warnings, so
we should decide now which we want to do. Is this a regular warning or a
deprecation warning?

On Sunday, August 21, 2016, Chris Dickinson notifications@github.com
wrote:

It's been a bit since I've been in the promises-in-core headspace, but,
IIRC, I believe where I settled after the discussion last spring was thus:

  1. We should definitely warn on unhandled rejection in the absence
    of other listeners by default for at least one major version.
  2. We should definitely fix the interaction between Promises and
    AsyncWrap/Domains.
  3. We should consider crashing on unhandled rejection in the absence
    of listeners in a subsequent major.

This PR addresses point (1) and, my nits around the wording of the error
aside, LGTM. Point (2) is tangential to this conversation, except for
release timing relating to point (3). Point (3) relates to this
conversation in that depending on the nature of the warning we log, we
could introduce the breaking behavior of unhandled rejection sooner — that
is, if we log a deprecation warning in v6, we could introduce
crash-on-unhandled-rejection in v7.

My question is: do we want to — or need to — make a decision on that in
advance of landing this PR? I note that with this change, we're taking a
subset of programs that would have previously been "quiet" and making them
noisy. This implicitly disincentivizes users from designing applications
and packages that rely on handling previously unhandled rejections. The
work necessary to correct the new logging behavior is the same as the work
necessary to correct the crashing behavior. My sense is that folks who
object to crashing on unhandled rejection may also object to warning on
unhandled rejection, but I would like to check that assumption.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8217 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eedtrz47vt_qdhtM_wgm_Bkuj59Oks5qiOyWgaJpZM4JpaK0
.

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Aug 22, 2016

  • We should definitely fix the interaction between Promises and AsyncWrap/Domains.

Point (2) is tangential to this conversation, except for release timing relating to point (3)

@fhinkel has been helping me get @trevnorris in touch with the V8 team about the MicroTaskQueue here: https://bugs.chromium.org/p/v8/issues/detail?id=4643

I definitely think we should float patches to expose what we need for the time being.

My question is: do we want to — or need to — make a decision on that in advance of landing this PR?

Maybe in regards to documentation?

If we are going to "crash", ideally this message would say that we will in the future. Documentation would also be helpful in warning of that in advance.

My sense is that folks who object to crashing on unhandled rejection may also object to warning on unhandled rejection, but I would like to check that assumption.

Probably. You win some, you loose some -- I think we are working within our boundaries here.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 22, 2016

Thinking about this a bit further... we could actually add a second, separate DeprecationWarning here that would only be printed on the first unhandled rejection.

@jasnell jasnell force-pushed the jasnell:warning-on-unhandled-rejection branch Aug 22, 2016

@jasnell jasnell added semver-major and removed semver-minor labels Aug 22, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 22, 2016

Added a third deprecation commit. The first two can easily be ported into v6 as a semver-minor, adding the regular warning that will be emitted on every unhandled rejection. The third is a semver-major that would go only into v7. It prints the one time deprecation warning on the first unhandled rejection.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 22, 2016

I'll work on a documentation update next.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Aug 22, 2016

Thanks for picking up my slack and sorry!

@benjamingr

This comment has been minimized.

Member

benjamingr commented Aug 22, 2016

Changes themselves LGTM

addPendingUnhandledRejection(promise, reason);
}
function rejectionHandled(promise) {
var hasBeenNotified = hasBeenNotifiedProperty.get(promise);
if (hasBeenNotified !== undefined) {
hasBeenNotifiedProperty.delete(promise);
const uid = promiseToGuidProperty.get(promise);

This comment has been minimized.

@yorkie

yorkie Aug 22, 2016

Member

Could we move this variable to define at line 36?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Aug 22, 2016

Added a third deprecation commit. The first two can easily be ported into v6 as a semver-minor, adding the regular warning that will be emitted on every unhandled rejection. The third is a semver-major that would go only into v7. It prints the one time deprecation warning on the first unhandled rejection.

@jasnell could we do that in a separate PR? Doing majors alongside non-majors is a recurring pain for releasers of the Current branch.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 22, 2016

Yep, absolutely. Alternatively I can simply put a backport pr together for
v6. Either way should work.

On Monday, August 22, 2016, Jeremiah Senkpiel notifications@github.com
wrote:

Added a third deprecation commit. The first two can easily be ported into
v6 as a semver-minor, adding the regular warning that will be emitted on
every unhandled rejection. The third is a semver-major that would go only
into v7. It prints the one time deprecation warning on the first unhandled
rejection.

@jasnell https://github.com/jasnell could we do that in a separate PR?
Doing majors alongside non-majors is a recurring pain for releasers of the
Current branch.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8217 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eQdkkX6y7u1OWR-eQ4Exdoh4RWopks5qiaD9gaJpZM4JpaK0
.

@Fishrock123

View changes

test/parallel/test-promises-warning-on-unhandled-rejection.js Outdated
var b = 0;
process.on('warning', common.mustCall((warning) => {
switch (b) {

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 22, 2016

Member

more robust to do b++ in the switch

@@ -0,0 +1,31 @@
// Flags: --no-warnings

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 22, 2016

Member

should be unnecessary since we are handling it?

This comment has been minimized.

@jasnell

jasnell Aug 22, 2016

Member

This prevents the unnecessary stderr output so the test isn't needlessly noisy.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 23, 2016

Member

@jasnell It should never reach stderr since we are listening to it. is there a bug?

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

That's not how emitWarning works. By default it prints to stderr and emits the 'warning' event. The --no-warnings switch suppresses the default stderr output but maintains the 'warning' event. There is no bug.

@jasnell jasnell force-pushed the jasnell:warning-on-unhandled-rejection branch 2 times, most recently Aug 22, 2016

@jasnell jasnell referenced this pull request Aug 22, 2016

Closed

promise: warning on unhandled rejection (v6.x) #8223

3 of 3 tasks complete
@benjamingr

This comment has been minimized.

Member

benjamingr commented Aug 23, 2016

Still LGTM.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 23, 2016

@chrisdickinson and @Fishrock123 ... PTAL. The commits have been updated with a more precise warning message.

@Fishrock123

View changes

lib/internal/process/promises.js Outdated
deprecationWarned = true;
process.emitWarning(
'Unhandled promise rejections are deprecated. In the future, ' +
'an error will be thrown when a promise rejection is not ' +

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 23, 2016

Member

No error will be thrown.

There is no way to catch it so that you cannot listen to GC.
It will exit with the rejection formatted how we format regular uncaught exceptions.

'In the future, the Node process will exit if promise rejections are not handled before garbage collection.'

That may be slightly confusing since this warning message is not on GC... not sure how to clarify.

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

process.emit('error') could be used to propagate the error but understood. Perhaps,

In the future, promise rejections that are not handled before garbage collection
will be handled as exceptions that may terminate the Node.js process.

That gives us some wiggle room in how we handle it later on.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 24, 2016

Member

@jasnell We won't do it if it must expose GC via a publicly accessible API. Legit everyone will yell at us.

If we do it, it will terminate the process.

Tangentially, it will also produce error output and a non-zero exit code if the process exits normally with an unhanded rejection that has not been GC'd (for completeness)

This comment has been minimized.

@benjamingr

benjamingr Aug 24, 2016

Member

'In the future, the Node process will exit with a non-zero exit code if a promise rejection is not handled and the promise is no longer referenced by anything.'

We can say what we mean by garbage collection without saying garbage collection which should be fine :)

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

@Fishrock123 ... mentioning garbage collection in the message does not imply that we have to expose garbage collection via a public API..

In the future, promise rejections that are not handled before garbage collection
will be handled as exceptions that will terminate the Node.js process with a
non-zero exit code.

This comment has been minimized.

@chrisdickinson

chrisdickinson Aug 24, 2016

Contributor

I'm confused by garbage collection entering the conversation here: my understanding is that we would crash on unhandled rejection as soon as it happened if an unhandledRejection listener were not installed.

This comment has been minimized.

@benjamingr

benjamingr Aug 24, 2016

Member

Any comments on my terminology? We can say not referenced without saying GC

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

Your suggested terminology is fine, I just think there's still some disagreement on exactly when the error should eventually happen.

This comment has been minimized.

@chrisdickinson

chrisdickinson Aug 24, 2016

Contributor

OK, I'm back up to date. The conversation about "when" the error crashes — whether on GC, or immediately — should probably be left for later, to see how the community at large reacts to the warning messages. If, by this time next year, there's still a lot of folks leaning on handling previously unhandled rejections, we can lean towards crashing on GC, otherwise we can crash immediately. To that end, a slight spin on @jasnell's wording:

In the future, promise rejections that are not handled will
terminate the Node.js process with a non-zero exit code.

(Without a specific "when" — whether at GC, or at unhandled-rejection-time)

This comment has been minimized.

@jasnell

jasnell Aug 24, 2016

Member

Works for me.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 24, 2016

On other thought on this... in the emitted warning, it might be a good idea to attach the uid as a standalone property on the warning object in addition to having it in the error message.

@jasnell jasnell force-pushed the jasnell:warning-on-unhandled-rejection branch Aug 24, 2016

jasnell added a commit to jasnell/node that referenced this pull request Oct 24, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [nodejs#8946](nodejs#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [nodejs#8169](nodejs#8169).
  * Passing a negative number to allocUnsafe will now throw an error [nodejs#7079](nodejs#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [nodejs#7399](nodejs#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [nodejs#3747](nodejs#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [nodejs#8317](nodejs#8317), [nodejs#8852](nodejs#8852), [nodejs#9253](nodejs#9253).
  * NODE_MODULE_VERSION has been updated to 51 [nodejs#8808](nodejs#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [nodejs#7897](nodejs#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [nodejs#8908](nodejs#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [nodejs#8217](nodejs#8217).
* Punycode
  * The `punycode` module has been deprecated [nodejs#7941](nodejs#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [nodejs#7448](nodejs#7448).

jasnell added a commit that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099

jasnell added a commit that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
Notable Changes:

* Buffer
  * Passing invalid input to Buffer.byteLength will now throw an error [#8946](#8946).
  * Calling Buffer without new is now deprecated and will emit a process warning [#8169](#8169).
  * Passing a negative number to allocUnsafe will now throw an error [#7079](#7079).
* Child Process
  * The fork and execFile methods now have stronger argument validation [#7399](#7399).
* Cluster
  * The worker.suicide method is deprecated and will emit a process warning [#3747](#3747).
* Deps
  * V8 has been updated to 5.4.500.36 [#8317](#8317), [#8852](#8852), [#9253](#9253).
  * NODE_MODULE_VERSION has been updated to 51 [#8808](#8808).
* File System
  * A process warning is emitted if a callback is not passed to async file system methods [#7897](#7897).
* Intl
  * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](#8908).
* Promises
  * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](#8217).
* Punycode
  * The `punycode` module has been deprecated [#7941](#7941).
* URL
  * An Experimental WHATWG URL Parser has been introduced [#7448](#7448).

PR-URL: #9099

imyller added a commit to imyller/meta-nodejs that referenced this pull request Oct 25, 2016

2016-10-25, Version 7.0.0 (Current)
    Notable Changes:

    * Buffer
      * Passing invalid input to Buffer.byteLength will now throw an error [#8946](nodejs/node#8946).
      * Calling Buffer without new is now deprecated and will emit a process warning [#8169](nodejs/node#8169).
      * Passing a negative number to allocUnsafe will now throw an error [#7079](nodejs/node#7079).
    * Child Process
      * The fork and execFile methods now have stronger argument validation [#7399](nodejs/node#7399).
    * Cluster
      * The worker.suicide method is deprecated and will emit a process warning [#3747](nodejs/node#3747).
    * Deps
      * V8 has been updated to 5.4.500.36 [#8317](nodejs/node#8317), [#8852](nodejs/node#8852), [#9253](nodejs/node#9253).
      * NODE_MODULE_VERSION has been updated to 51 [#8808](nodejs/node#8808).
    * File System
      * A process warning is emitted if a callback is not passed to async file system methods [#7897](nodejs/node#7897).
    * Intl
      * Intl.v8BreakIterator constructor has been deprecated and will emit a process warning [#8908](nodejs/node#8908).
    * Promises
      * Unhandled Promise rejections have been deprecated and will emit a process warning [#8217](nodejs/node#8217).
    * Punycode
      * The `punycode` module has been deprecated [#7941](nodejs/node#7941).
    * URL
      * An Experimental WHATWG URL Parser has been introduced [#7448](nodejs/node#7448).

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
@vitaly-t

This comment has been minimized.

Contributor

vitaly-t commented Nov 1, 2016

Will the generic error handler be usable for unhandled rejections?

@Fishrock123

This comment has been minimized.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Nov 2, 2016

@vitaly-t at the moment - a warning is emitted and you need to add an unhandledRejection event handler separately. In the future GC based event handling will be performed.

I intend to open a PR clarifying the wording of unhandled rejections soon (I'm between laptops :( )

@leodutra

This comment has been minimized.

leodutra commented Nov 11, 2016

Thank you so much for this. The other behavior was a kick in the balls.

@danShumway

This comment has been minimized.

danShumway commented Nov 24, 2016

Highly approve of this change.

If people aren't explicitly handling errors, it's easy for promises to eat them which makes debugging much harder, since there's no easy way to trace which promise has forgotten its error callback or what the original error was in the first place.

What is the timeline for actually removing the behavior though? I have libraries where I'd like errors within callbacks or methods to actually break things like they'd normally do - specifically in situations where I don't want to necessarily force the user to think about the fact that they're working with Promises behind the scenes.

I'm trying to decide if I can just leave things as they are in anticipation of the full removal, or if I should try to polyfill a similar behavior in the meantime.

constructPromise().catch(function (err) {
    console.log(err);
    process.exit(1);
});
@benjamingr

This comment has been minimized.

Member

benjamingr commented Dec 1, 2016

What is the timeline for actually removing the behavior though? I have libraries where I'd like errors within callbacks or methods to actually break things like they'd normally do - specifically in situations where I don't want to necessarily force the user to think about the fact that they're working with Promises behind the scenes.

Either use a library like bluebird and add a onPossiblyUnhandledRejection for only your promises (which would not cause issues) or if it's your app add a "unhandledRejection" event handler on process.

As for the actual change - not really sure where it stands with @Fishrock123 but IIRC there was an issue with V8 being addressed.

mbland added a commit to mbland/hubot-slack-github-issues that referenced this pull request Dec 1, 2016

scripts: Always convert result to resolved Promise
As discovered in 18F#51, the
`UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning`
warnings were apparently added in v6.6.0
(https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added
by nodejs/node#8223. See also:

  nodejs/node#6439
  nodejs/node#8217
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning

Test failures from `test/integration-test.js` after upgrading to Node
v6.9.1 showed extra output such as:

```
  (node:39696) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): null
  (node:39696) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 2)
```

This was happening because `scripts/slack-github-issues.js` created a
Hubot listener that returned a `Promise` so that the integration test
could use `.should.be.rejectedWith` assertions. Rather than jump through
hoops to quash the warnings, this change now causes the listener's
`catch` handler to return the result of the rejected `Promise`,
converting it to a fulfilled `Promise` in the process.

Since Hubot never used the result anyway, the only effect it has in
production is to eliminate the warning messages. The tests now just
check that the `Promise` returned by the listener callback is fulfilled
with the expected error result, with practically no loss in clarity.

mbland added a commit to mbland/unit-testing-node that referenced this pull request Apr 28, 2017

Fix Node v6.6.0+ UnhandledPromiseRejectionWarning
Backported from mbland/hubot-slack-github-issues#7.

As discovered in 18F/hubot-slack-github-issues#51, the
`UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning`
warnings were apparently added in v6.6.0
(https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added
by nodejs/node#8223. See also:

  nodejs/node#6439
  nodejs/node#8217
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning

A test failure from `solutions/06-integration/test/integration-test.js`
after upgrading to Node v6.9.1 showed output such as:

```
-  "(node:87412) UnhandledPromiseRejectionWarning: Unhandled
     promise rejection (rejection id: 14): Error: failed to create a
     GitHub issue in 18F/handbook: received 500 response from GitHub
     API: {\"message\":\"test failure\"}\n"
```

This was happening because `scripts/slack-github-issues.js`
ignored the return value from `Middleware.execute()`, whether it was
undefined or a `Promise`. For consistency's sake (and to provide a
clearer upgrade path to the current state of
mbland/slack-github-issues), `Middleware.execute()` always returns a
`Promise`, and `scripts/slack-github-issues.js` handles and ignores any
rejected Promises.

This fixed the `integration-test.js` error, but also required minor
updates to `solutions/{05-middleware,complete}/test/middleware-test.js`.
The next commit will update the tutorial narrative to account for this
change.

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete

@benjamingr benjamingr referenced this pull request Apr 25, 2018

Open

src: exit on gc unhandled rejection #20097

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