fix(instrumentation-router): prevent MaxListenersExceededWarning on response stream - #3637
Open
bhuvan-somisetty wants to merge 1 commit into
Open
Conversation
github-actions
Bot
requested review from
JamieDanielson,
pkanal and
raphael-theriault-swi
July 23, 2026 15:30
Pull request dashboard statusWaiting on reviewers · refreshed 2026-08-10 13:49 UTC Review the latest changes. Status above doesn't look right?
|
Contributor
|
This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. |
pichlermarc
requested changes
Jul 31, 2026
Comment on lines
+30
to
181
| // `glob` always wants forward slashes in its patterns, even on Windows, | ||
| // where `path.join` produces backslashes. | ||
| function toGlobPattern(...parts) { | ||
| return path | ||
| .join(...parts) | ||
| .split(path.sep) | ||
| .join('/'); | ||
| } | ||
|
|
||
| let numProbs = 0; | ||
| function problem(...args) { | ||
| numProbs += 1; | ||
| if (USE_COLOR) { | ||
| process.stdout.write('\x1b[31m'); | ||
| } | ||
| args.unshift('bitrot error:'); | ||
| console.log(...args); | ||
| if (USE_COLOR) { | ||
| process.stdout.write('\x1b[39m'); | ||
| } | ||
| } | ||
|
|
||
| function warn(...args) { | ||
| if (USE_COLOR) { | ||
| process.stdout.write('\x1b[33m'); | ||
| } | ||
| args.unshift('bitrot warn:'); | ||
| console.warn(...args); | ||
| if (USE_COLOR) { | ||
| process.stdout.write('\x1b[39m'); | ||
| } | ||
| } | ||
|
|
||
| function gitCloneSync(repo, dir) { | ||
| execSync(`git clone ${repo} "${dir}"`); | ||
| } | ||
| function gitPullSync(cwd) { | ||
| execSync('git pull', { cwd }); | ||
| } | ||
|
|
||
| function isPublicPackage(pj) { | ||
| if (pj.private === true) { | ||
| return false; | ||
| } else if (pj.publishConfig?.access) { | ||
| return pj.publishConfig.access === 'public'; | ||
| } else { | ||
| // Default is *false* for scoped packages. | ||
| return !pj.name.startsWith('@'); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * BITROT: Check that the `"groupName": "OTel Core experimental",` | ||
| * group in renovate.json matches the actual current set of experimental | ||
| * packages from the core repo. | ||
| */ | ||
| function bitrotRenovateCoreExperimental() { | ||
| const renovateJson = path.join(TOP, 'renovate.json'); | ||
| const renovate = JSON.parse(fs.readFileSync(renovateJson)); | ||
| const group = renovate.packageRules.filter( | ||
| r => r.groupName === 'OTel Core experimental' | ||
| )[0]; | ||
| assert.ok(group, `found "OTel Core experimental" group in ${renovateJson}`); | ||
|
|
||
| const ojDir = path.join(BUILD_DIR, 'opentelemetry-js'); | ||
| if (fs.existsSync(ojDir)) { | ||
| gitPullSync(ojDir); | ||
| } else { | ||
| gitCloneSync( | ||
| 'https://github.com/open-telemetry/opentelemetry-js.git', | ||
| ojDir | ||
| ); | ||
| } | ||
|
|
||
| const pkgNames = globSync( | ||
| path.join(ojDir, 'experimental/packages/*/package.json') | ||
| toGlobPattern(ojDir, 'experimental/packages/*/package.json') | ||
| ) | ||
| .map(packageJson => JSON.parse(fs.readFileSync(packageJson))) | ||
| .filter(pj => isPublicPackage(pj)) | ||
| .map(pj => pj.name); | ||
|
|
||
| const pkgNamesSet = new Set(pkgNames); | ||
| const renovateSet = new Set(group.matchPackageNames); | ||
| const missing = pkgNamesSet.difference(renovateSet); // requires Node.js >=22 | ||
| const extraneous = renovateSet.difference(pkgNamesSet); | ||
|
|
||
| const issues = []; | ||
| if (missing.size) { | ||
| issues.push(`missing entries: ${JSON.stringify(Array.from(missing))}`); | ||
| } | ||
| if (extraneous.size) { | ||
| issues.push( | ||
| `extraneous entries: ${JSON.stringify(Array.from(extraneous))}` | ||
| ); | ||
| } | ||
| if (issues.length) { | ||
| problem( | ||
| `${renovateJson}: "matchPackageNames" in the "OTel Core experimental" group does not match the current set experimental packages from the opentelemetry-js.git repo:\n - ${issues.join( | ||
| '\n - ' | ||
| )}\nThe "matchPackageNames" should be:\n${JSON.stringify( | ||
| pkgNames.sort(), | ||
| null, | ||
| 2 | ||
| )}` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| function getNpmInfo(name) { | ||
| const CACHE_TIMEOUT_MS = 30 * 60 * 1000; // 30 minutes | ||
| const cache = ensureCacheLoaded('npmInfo'); | ||
| const cacheEntry = cache[name]; | ||
| if (cacheEntry) { | ||
| if (cacheEntry.timestamp + CACHE_TIMEOUT_MS > Date.now()) { | ||
| return cacheEntry.value; | ||
| } else { | ||
| delete cache[name]; | ||
| } | ||
| } | ||
|
|
||
| // Limited security guard on exec'ing given `name`. | ||
| const PKG_NAME_RE = /^(@[\w_.-]+\/)?([\w_.-]+)$/; | ||
| if (!PKG_NAME_RE.test(name)) { | ||
| throw new Error( | ||
| `${JSON.stringify(name)} does not look like a valid npm package name` | ||
| ); | ||
| } | ||
|
|
||
| const stdout = execSync(`npm info --json "${name}"`); | ||
| const npmInfo = JSON.parse(stdout); | ||
|
|
||
| cache[name] = { | ||
| timestamp: Date.now(), | ||
| value: npmInfo, | ||
| }; | ||
| saveCache(); | ||
| return npmInfo; | ||
| } | ||
|
|
||
| /** | ||
| * BITROT: Check if instrumentations are missing support for major releases of | ||
| * the target package. | ||
| * | ||
| * Limitations: | ||
| * - This doesn't currently handle both `pg` and `pg-pool` from instr-pg. | ||
| * - This doesn't currently support instr-aws-sdk, because of the wildcard in | ||
| * the packages supported: `@aws-sdk/client-*`. | ||
| */ | ||
| function bitrotInstrumentations() { | ||
| const instrReadmes = globSync( | ||
| path.join(TOP, 'packages/instrumentation-*/README.md') | ||
| toGlobPattern(TOP, 'packages/instrumentation-*/README.md') | ||
| ); |
Member
There was a problem hiding this comment.
please remove these unrelated changes.
Contributor
Author
There was a problem hiding this comment.
Hi @pichlermarc, thanks for catching that! I've rebased the branch on latest main and removed the unrelated changes.
bhuvan-somisetty
force-pushed
the
fix/router-stream-max-listeners-3547
branch
from
July 31, 2026 16:10
011decc to
5a57b29
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which problem is this PR solving?
Fixes #3547
When using streams or async handlers with
@opentelemetry/instrumentation-routerand@opentelemetry/instrumentation-express, each pending layer registers acloselistener on the response object viaensureFallbackListener(). When multiple async handlers or stream events stack up, Node emits aMaxListenersExceededWarning: Possible EventEmitter memory leak detectedbecauseres.getMaxListeners()is exceeded.Short description of the changes
res.getMaxListeners()by 1 whenever attaching a fallbackcloselistener in both@opentelemetry/instrumentation-routerand@opentelemetry/instrumentation-express.res.getMaxListeners()back down when thecloseevent fires or whenremoveListener('close', ...)is called viawrappedNext/ callback completion.instrumentation-routerverifying that async middleware with streaming responses and pre-populated listeners do not triggerMaxListenersExceededWarning.Type of change
How Has This Been Tested?
Added unit tests in
packages/instrumentation-router/test/index.test.tscovering async middleware and response streaming scenarios to assert noMaxListenersExceededWarningis emitted.Checklist