Skip to content
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

events: implement EventEmitter#getMaxListeners() #82

Closed
wants to merge 1 commit into from

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Dec 5, 2014

Returns the current max listener value for the emitter which is either set by
`emitter.setMaxListeners(n)` or defaults to `EventEmitter.defaultMaxListeners`.

This can be usefull to increment/decrement max listeners to avoid the warning
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/usefull/useful

Maybe clarify what warning? I know the warning but might be better to specify.

@tellnes
Copy link
Contributor Author

tellnes commented Dec 5, 2014

@kenany Thanks for feedback. I've addressed your issues.

About clarifying the warning. Yes, I think that should be done, but I think it is out of scope for this pull request.

@@ -67,6 +67,14 @@ EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) {
return this;
};

EventEmitter.prototype.getMaxListeners = function getMaxListeners() {
if (!util.isUndefined(this._maxListeners)) {
return this._maxListeners;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about returning Infinity when this._maxListeners is 0 to make more semantic, actually setMaxListeners(Infinity) is working well, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking about that. It will return Infinity if you have done setMaxListeners(Infinity). But it is documented that 0 means unlimited and I think being concise with existing api is best.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, yup, but the Infinity in fact is better, because both setMaxListeners(Infinity) and setMaxListeners(0) are working always. perhaps any thoughts from TC?

@bnoordhuis
Copy link
Member

Some style issues but otherwise LGTM. I'd like one more person to sign off on this.

@tellnes
Copy link
Contributor Author

tellnes commented Dec 5, 2014

I've fixed all comments so far. The only thing missing is if it should return Infinity if the value is zero as proposed by @yorkie in an outdated diff.

@bnoordhuis
Copy link
Member

I would just make it return zero. Intuitively, I would expect ee.setMaxListeners(0).getMaxListeners() to return zero, not infinity. Principle of Least Surprise and all that.

I'll land this PR assuming tests pass. If someone feels strongly about the return value, they can follow up with a PR of their own.

@chrisdickinson
Copy link
Contributor

LGTM.

@bnoordhuis
Copy link
Member

Landed in 0115a5e. Thanks, Christian!

@bnoordhuis bnoordhuis closed this Dec 5, 2014
bnoordhuis pushed a commit that referenced this pull request Dec 5, 2014
@bnoordhuis
Copy link
Member

Sorry about that, I screwed up the commit. 2931348 it is.

vlomovtsev pushed a commit to auriga-foss/node that referenced this pull request Feb 14, 2023
…recompiled_vfsVfsNet

While working with NodeJS tests it was found that some of them (tests)
requires access to /etc/hosts file and so the getaddrinfo() call falls
to error.

The quote from developers:
> The issue was nailed down to the ramdisk access for the entity which
> implements network access (precompiled_vfsVfsNet). As appropriated
> partition hasn't been mounted for it (so it is not exists for it). The
> access to the ramdisk is covered by another entity, called
> precompiled_vfsVfsRamFs. So it is required to configure (allow) access
> for the precompiled_vfsVfsNet to the ramdisk over explicit properties.
>
> Besides of that it is required to add the 'hosts' file to the romfs
> image, to make it accessible by application over /etc mount from romfs.

TFS: nodejs#82

Signed-off-by: Vadim Lomovtsev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants