events: make memory leak warning more programatically accessible #8298

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@addaleax
Member

addaleax commented Aug 27, 2016

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

events

Description of change

This makes the famous EventEmitter memory leak warnings occurring when the listener count for a given event exceeds a specified number more programatically accessible, by giving them properties referring to the event emitter instance and the event itself.

This can be useful for debugging the origins of such a warning when the stack itself doesn’t reveal enough information about the event emitter instance itself, e.g. when manual inspection of the
already-registered listeners is expected to be useful.

(It seems CI on this has to wait until Jenkins is running smoothly again…)
CI: https://ci.nodejs.org/job/node-test-commit/4794/

events: make memory leak warning more accessible
This makes the famous `EventEmitter memory leak` warnings occurring
when the listener count for a given event exceeds a specified number
more programatically accessible, by giving them properties referring
to the event emitter instance and the event itself.

This can be useful for debugging the origins of such a warning when
the stack itself doesn’t reveal enough information about the event
emitter instance itself, e.g. when manual inspection of the
already-registered listeners is expected to be useful.
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 27, 2016

Contributor

LGTM

Contributor

cjihrig commented Aug 27, 2016

LGTM

`${existing.length} ${type} listeners added. ` +
'Use emitter.setMaxListeners() to increase limit');
+ w.name = 'Warning';

This comment has been minimized.

@jasnell

jasnell Aug 27, 2016

Member

Assigning a specific warning name would make differentiating a bit easier as well.

@jasnell

jasnell Aug 27, 2016

Member

Assigning a specific warning name would make differentiating a bit easier as well.

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

+1 … but that would be a semver-major change?

@addaleax

addaleax Aug 27, 2016

Member

+1 … but that would be a semver-major change?

This comment has been minimized.

@jasnell

jasnell Aug 27, 2016

Member

Yeah, good point. Nevermind then

On Saturday, August 27, 2016, Anna Henningsen notifications@github.com
wrote:

In lib/events.js
#8298 (comment):

                         `${existing.length} ${type} listeners added. ` +
                         'Use emitter.setMaxListeners() to increase limit');
  •    w.name = 'Warning';
    

+1 … but that would be a semver-major change?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/8298/files/802e6be4c7e2bfd08224fb6769fbe7b2d8ed0239#r76515949,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eW6tZ7CDeSmG54J9RAysM9fS9TBZks5qkDgHgaJpZM4Jupwl
.

@jasnell

jasnell Aug 27, 2016

Member

Yeah, good point. Nevermind then

On Saturday, August 27, 2016, Anna Henningsen notifications@github.com
wrote:

In lib/events.js
#8298 (comment):

                         `${existing.length} ${type} listeners added. ` +
                         'Use emitter.setMaxListeners() to increase limit');
  •    w.name = 'Warning';
    

+1 … but that would be a semver-major change?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/pull/8298/files/802e6be4c7e2bfd08224fb6769fbe7b2d8ed0239#r76515949,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eW6tZ7CDeSmG54J9RAysM9fS9TBZks5qkDgHgaJpZM4Jupwl
.

+have the additional `emitter`, `type` and `count` properties, referring to
+the event emitter instance, the event’s name and the number of attached
+listeners, respectively.
+

This comment has been minimized.

@jasnell

jasnell Aug 27, 2016

Member

May be worth also reiterating that --trace-warnings will print out the stack trace detail.

@jasnell

jasnell Aug 27, 2016

Member

May be worth also reiterating that --trace-warnings will print out the stack trace detail.

This comment has been minimized.

@addaleax

addaleax Aug 27, 2016

Member

@jasnell good idea, done!

@addaleax

addaleax Aug 27, 2016

Member

@jasnell good idea, done!

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 27, 2016

Member

Couple of suggestions but otherwise LGTM

Member

jasnell commented Aug 27, 2016

Couple of suggestions but otherwise LGTM

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 27, 2016

Member

LGTM if CI is green

Member

Fishrock123 commented Aug 27, 2016

LGTM if CI is green

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 30, 2016

Member

Landed in 932c824

Member

addaleax commented Aug 30, 2016

Landed in 932c824

@addaleax addaleax closed this Aug 30, 2016

addaleax added a commit that referenced this pull request Aug 30, 2016

events: make memory leak warning more accessible
This makes the famous `EventEmitter memory leak` warnings occurring
when the listener count for a given event exceeds a specified number
more programatically accessible, by giving them properties referring
to the event emitter instance and the event itself.

This can be useful for debugging the origins of such a warning when
the stack itself doesn’t reveal enough information about the event
emitter instance itself, e.g. when manual inspection of the
already-registered listeners is expected to be useful.

PR-URL: #8298
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com>

@addaleax addaleax deleted the addaleax:events-warning branch Aug 30, 2016

addaleax added a commit to addaleax/node that referenced this pull request Aug 30, 2016

events: make memory leak warning name more verbose
Switch from a generic `Warning` to the more specific
`MaxListenersExceededWarning`.

Ref: nodejs#8298

@addaleax addaleax referenced this pull request Aug 30, 2016

Closed

events: make memory leak warning name more verbose #8341

4 of 4 tasks complete

addaleax added a commit that referenced this pull request Sep 4, 2016

events: make memory leak warning name more verbose
Switch from a generic `Warning` to the more specific
`MaxListenersExceededWarning`.

Ref: #8298
PR-URL: #8341
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

@Fishrock123 Fishrock123 referenced this pull request Sep 6, 2016

Closed

v6.6.0 pre-proposal #8428

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 8, 2016

events: make memory leak warning more accessible
This makes the famous `EventEmitter memory leak` warnings occurring
when the listener count for a given event exceeds a specified number
more programatically accessible, by giving them properties referring
to the event emitter instance and the event itself.

This can be useful for debugging the origins of such a warning when
the stack itself doesn’t reveal enough information about the event
emitter instance itself, e.g. when manual inspection of the
already-registered listeners is expected to be useful.

PR-URL: nodejs#8298
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com>

Fishrock123 added a commit that referenced this pull request Sep 9, 2016

events: make memory leak warning more accessible
This makes the famous `EventEmitter memory leak` warnings occurring
when the listener count for a given event exceeds a specified number
more programatically accessible, by giving them properties referring
to the event emitter instance and the event itself.

This can be useful for debugging the origins of such a warning when
the stack itself doesn’t reveal enough information about the event
emitter instance itself, e.g. when manual inspection of the
already-registered listeners is expected to be useful.

PR-URL: #8298
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <Fishrock123@rocketmail.com>

@Fishrock123 Fishrock123 referenced this pull request Sep 9, 2016

Merged

v6.6.0 proposal #8466

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

2016-09-14, Version 6.6.0 (Current)
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) #8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) #8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) #8174

Refs: #8428
Refs: #8457
PR-URL: #8466

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 15, 2016

2016-09-14, Version 6.6.0 (Current)
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
nodejs#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) nodejs#8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
nodejs#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
nodejs#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) nodejs#8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) nodejs#8174

Refs: nodejs#8428
Refs: nodejs#8457
PR-URL: nodejs#8466

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

2016-09-14, Version 6.6.0 (Current)
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

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

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

2016-09-14, Version 6.6.0 (Current)
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment