Skip to content

feat: add router cycle detection to prevent infinite loops#183

Open
aprici7y wants to merge 1 commit intopillarjs:masterfrom
aprici7y:fix/router-cycle-detection
Open

feat: add router cycle detection to prevent infinite loops#183
aprici7y wants to merge 1 commit intopillarjs:masterfrom
aprici7y:fix/router-cycle-detection

Conversation

@aprici7y
Copy link
Copy Markdown

@aprici7y aprici7y commented Oct 7, 2025

Detects when a router is mounted on itself (directly or indirectly) and emits a warning to help developers identify cycles that cause requests to hang indefinitely.

When a router is mounted on itself through a chain of routers, requests that traverse the cycle hang indefinitely and eventually cause a stack overflow. This is difficult to debug because the application appears to work fine until a request hits the cyclic path.

This implements cycle detection in Router.prototype.use that:

  • Detects both direct self-mounts (router.use(router)) and indirect cycles (routerA -> routerB -> routerA)
  • Uses recursive traversal with a visited set to prevent infinite loops during detection
  • Emits a process.emitWarning() with type RouterCycleWarning
  • Maintains backward compatibility - the mount still succeeds but with a warning

Manual Test

const Router = require('router')

// Direct self-mount
const loop = new Router()
loop.use('/', loop) // Emits RouterCycleWarning

// Indirect cycle
const routerA = new Router()
const routerB = new Router()
routerA.use('/b', routerB)
routerB.use('/a', routerA) // Emits RouterCycleWarning

Fixes expressjs/express#6809

Router cycles cause silent runtime hangs when a router is mounted
on itself (directly or indirectly). This change detects such cycles
and emits a warning to help developers identify the issue quickly.

The implementation:
- Detects cycles in Router.prototype.use before mounting
- Uses recursive traversal with visited set to avoid infinite loops
- Detects both direct self-mounts and indirect cycles
- Emits process warning but keeps behavior unchanged (warn-only)
- Can be turned into an error in future major version

Tests added for:
- Direct self-loop (router.use(router))
- Indirect cycle (routerA -> routerB -> routerA)
- Nested indirect cycles (routerA -> routerB -> routerC -> routerA)
- Ensuring non-cyclic mounts don't trigger warnings
- Verifying behavior remains unchanged (mount succeeds with warning)

Related to expressjs/express#6809
* @private
*/

function detectRouterCycle (parentRouter, childRouter, visited) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You run this function each time, so why not adding a Set/Map to avoid repetitive code?

@som14062005
Copy link
Copy Markdown

som14062005 commented Mar 31, 2026

I've opened a fresh PR addressing the same issue with a cleaner implementation: #193

Implementation Details

The fix adds a hasCycle() helper function that performs a depth-first
search (DFS) through the router tree before mounting any router instance.

  • In Router.prototype.use, before pushing a layer onto the stack,
    we check if the handler is a Router instance
  • If it is, hasCycle() walks the entire subtree of that router using
    a visited Set to avoid infinite loops during the check itself
  • If a cycle is detected, process.emitWarning() is called with code
    ROUTER_CYCLE_DETECTED, giving developers an immediate, actionable warning
  • If no cycle exists, everything proceeds as normal — zero performance
    impact on the happy path

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.

Router cycles cause silent runtime hangs

3 participants