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

Listeners for SIGINT (and other) signals are not registered in EventEmitter #22892

Closed
rafaelcalpena opened this issue Mar 13, 2024 · 0 comments · Fixed by #23890
Closed

Listeners for SIGINT (and other) signals are not registered in EventEmitter #22892

rafaelcalpena opened this issue Mar 13, 2024 · 0 comments · Fixed by #23890
Labels
bug Something isn't working node compat

Comments

@rafaelcalpena
Copy link

Version: Deno 1.41.2

Description

Deno correctly registers SIGINT and other SIG* signals with process.on('SIGINT') via node:process imports, but the listeners are not appended to process.listeners('SIGINT') return array. This differs from the original Node.js implementation behavior and causes issues with libraries such as signal-exit@3.0.7

Reproduction Steps

import process from "node:process";

process.on('customEvent', () => console.log('customEvent'))
console.log(process.listeners('customEvent')) // outputs [ [Function (anonymous)] ]

process.on('SIGINT', () => console.log('SIGINT'))
console.log(process.listeners('SIGINT')) // outputs [], but it should also contain an anonymous function 

setTimeout(() => {}, 10000)

Explanation

Since Deno uses a custom implementation for handling OS Signals, a specific condition was added to the process polyfill to redirect all process.on() calls for SIG* signals to use Deno.addSignalListener(). However, the method responsible for adding the listener to the array is located at super.on() (in the EventEmitter class), which is never called.

Possible Fix

One possible fix would be to add the super.on(event, listener); call right after the Deno.addSignalListener() line, but I am not sure whether this would cause unintended side-effects between Deno and the polyfill.

Thanks for building Deno!

@marvinhagemeister marvinhagemeister added bug Something isn't working node compat labels May 18, 2024
marvinhagemeister added a commit that referenced this issue May 20, 2024
Some npm libraries like `signal-exit` rely on the length of the listener
array returned by `process.listeners("SIGNT")` to be correct to
function. We weren't tracking `SIG*` events there, which broke those npm
libraries.

Fixes #22892
bartlomieju pushed a commit that referenced this issue May 21, 2024
Some npm libraries like `signal-exit` rely on the length of the listener
array returned by `process.listeners("SIGNT")` to be correct to
function. We weren't tracking `SIG*` events there, which broke those npm
libraries.

Fixes #22892
bartlomieju pushed a commit that referenced this issue May 21, 2024
Some npm libraries like `signal-exit` rely on the length of the listener
array returned by `process.listeners("SIGNT")` to be correct to
function. We weren't tracking `SIG*` events there, which broke those npm
libraries.

Fixes #22892
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants