Skip to content

Only add one process.exit handler#179

Closed
benlowry wants to merge 1 commit intobubenshchykov:masterfrom
benlowry:patch-1
Closed

Only add one process.exit handler#179
benlowry wants to merge 1 commit intobubenshchykov:masterfrom
benlowry:patch-1

Conversation

@benlowry
Copy link

If you create short-lived tunnels then each time it binds a method to the process.on('exit') until error messages start being emitted -

(node:17328) MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 exit listeners added to [process]. Use emitter.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)

@bubenshchykov
Copy link
Owner

bubenshchykov commented Jun 14, 2020

Thanks for reporting!

There is a guard that prevents spawning two processes in parallel https://github.com/bubenshchykov/ngrok/blob/master/process.js#L18

Thus, we can be assigning only one process.on('exit', .. ) handler at a time.

The use-case that you describe is: start process; kill process; start process; kill process - true, it will lead to multiple exit handlers and a memory leak.

So the problem boils down to simply calling process.removeListener(cb) inside exit callback, what do you think?

philnash added a commit to philnash/ngrok that referenced this pull request Jul 26, 2021
Also, stop performing an asynchronous action on process exit. It is not officially supported. However subprocess.kill is not asynchronous, so can be used over the internal killProcess method.

Fixes bubenshchykov#239. Should help bubenshchykov#231. Supersedes bubenshchykov#179
@philnash
Copy link
Collaborator

Thanks for this, I'm closing in favour of #240, in which we definitely only set up the listener once and don't need to introduce more state. I wouldn't have thought of it without this PR as inspiration though.

@philnash philnash closed this Jul 26, 2021
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.

3 participants