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

Proxy process.on("exit") to avoid MaxListenersExceededWarning #42

Merged
merged 9 commits into from Apr 14, 2021

Conversation

mike-marcacci
Copy link
Owner

This fixes #30, working around node's MaxListenersExceededWarning by creating a "proxy" event emitter to forward exit events.

This is a more concise alternative to #34.

@gabegorelick
Copy link

@mike-marcacci Won't you now just get the MaxListenersExceededWarning on the processExitProxy object instead of on process? That's somewhat better since you can call setMaxListeners(Infinity) on this proxy object, but I don't see that in this PR.

@mike-marcacci
Copy link
Owner Author

@gabegorelick 🤦🏼 indeed haha; that is now fixed in the PR.

This fixes #30, working around node's MaxListenersExceededWarning by creating a "proxy" event emitter to forward `exit` events.

This is a more concise alternative to #34.
CHANGELOG.md Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@jaydenseric
Copy link
Collaborator

@mike-marcacci may I suggest doing regular commits when working on the PR, vs force-pushes? That way the email notifications, PR history, etc. has the changes as incremental diffs which is easier to follow. You can always squash the PR to a single commit when merging it if you prefer a single commit on master afterwards.

mike-marcacci and others added 6 commits April 13, 2021 20:15
Co-authored-by: Jayden Seric <me@jaydenseric.com>
Co-authored-by: Jayden Seric <me@jaydenseric.com>
Co-authored-by: Jayden Seric <me@jaydenseric.com>
Co-authored-by: Jayden Seric <me@jaydenseric.com>
Co-authored-by: Jayden Seric <me@jaydenseric.com>
Co-authored-by: Jayden Seric <me@jaydenseric.com>
@mike-marcacci
Copy link
Owner Author

@jaydenseric yes indeed, that would probably be helpful here. I'm in the habit of staging larger PRs with distinct, logical commits (as opposed to "historical" commits), which can then be reviewed more easily. But for smaller PRs, yes I definitely agree: keeping the history greatly helps collaboration 👍🏼

src/index.ts Outdated Show resolved Hide resolved
Originally, when supporting node 8, we could not use `on` `off` and `once` methods on streams (because they did not exist). Accordingly, I chose to use `addListener` and `removeListener` both on streams and events, for consistency. With support for that version long-gone, we can now use the prefered methods.
Copy link
Collaborator

@jaydenseric jaydenseric left a comment

Choose a reason for hiding this comment

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

I don’t have the bandwidth right now to go too deep into reviewing this, but the approach looks good to me.

@mike-marcacci mike-marcacci merged commit a0d7b6c into master Apr 14, 2021
@jaydenseric
Copy link
Collaborator

jaydenseric commented Apr 14, 2021

It would be absurd, but I wonder what would happen if you constructed a new fs-capacitor ReadStream after the process exit event has been fired, say, within an exit event handler.

@mike-marcacci
Copy link
Owner Author

That's a good question with a pretty straightforward answer actually: nothing 🙃

Node will not schedule another event frame when this event is fired, and so only synchronous code gets executed in these handlers.

@mike-marcacci mike-marcacci deleted the event_proxy branch May 16, 2021 01:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MaxListenersExceededWarning
3 participants