Skip to content

refactor: cleanup signal listeners and auto-remove handlers#4

Merged
BQXBQX merged 1 commit into
hustcc:mainfrom
pchuri:cleanup/signal-listeners
Jul 18, 2025
Merged

refactor: cleanup signal listeners and auto-remove handlers#4
BQXBQX merged 1 commit into
hustcc:mainfrom
pchuri:cleanup/signal-listeners

Conversation

@pchuri

@pchuri pchuri commented Jul 17, 2025

Copy link
Copy Markdown
Contributor

Summary

Replace global setMaxListeners(0) with explicit management of SIGINT/SIGTERM handlers.

Changes

  • Use process.once('SIGINT', …) in createServer() to attach only a one-time shutdown handler.
  • In createBaseHttpServer(), remove SIGINT/SIGTERM handlers on server close to prevent listener accumulation.
  • Remove the previous process.setMaxListeners(0) hack.

This prevents the MaxListenersExceededWarning while keeping listener leaks in check.

Replace global setMaxListeners(0) with explicit SIGINT/SIGTERM listener management:
- use process.once in createServer()
- remove handlers on httpServer close in setupCleanupHandlers()
@pchuri pchuri marked this pull request as ready for review July 17, 2025 01:44
@withsmilo

Copy link
Copy Markdown
Contributor

Hi @hustcc .
Could you please merge this PR and then make a new release of mcp-mermaid? I feel this is so powerful MCP for all users!

@BQXBQX

BQXBQX commented Jul 17, 2025

Copy link
Copy Markdown
Collaborator

Summary

Replace global setMaxListeners(0) with explicit management of SIGINT/SIGTERM handlers.将全局 setMaxListeners(0) 替换为 SIGINT/SIGTERM 处理程序的显式管理。

Changes  变化

  • Use process.once('SIGINT', …) in createServer() to attach only a one-time shutdown handler.在 createServer() 中使用 process.once('SIGINT', ...) 仅附加一次性关闭处理程序。
  • In createBaseHttpServer(), remove SIGINT/SIGTERM handlers on server close to prevent listener accumulation.在 createBaseHttpServer() 中,删除 server close 上的 SIGINT/SIGTERM 处理程序,以防止侦听器累积。
  • Remove the previous process.setMaxListeners(0) hack.删除之前的 process.setMaxListeners(0) hack。

This prevents the MaxListenersExceededWarning while keeping listener leaks in check.这可以防止 MaxListenersExceededWarning,同时保持侦听器泄漏。

Can you elaborate on the memory leak you encountered? Won't process.exit(0) automatically clear the listener after the process terminates? Why do I need to remove it again? What logic causes listener accumulation?

@pchuri

pchuri commented Jul 17, 2025

Copy link
Copy Markdown
Contributor Author

Hi @BQXBQX, thanks for digging in on this – here’s a bit more context on why we needed to explicitly remove the SIGINT/SIGTERM handlers:


1) The warning that tipped me off

I ran into this exact warning when running in watch mode (where the server is restarted repeatedly):

(node:1) MaxListenersExceededWarning: Possible EventEmitter memory leak detected.
11 SIGINT listeners added to [process]. MaxListeners is 10.
Use emitter.setMaxListeners() to increase limit

Because createServer() (and under the hood createBaseHttpServer()) was called over and over (e.g. on each file-change in watch mode), each invocation registered a new SIGINT handler without ever removing the old one. Once you cross the default limit of 10 listeners, Node.js emits that MaxListenersExceededWarning.


2) Why process.exit(0) alone isn’t enough

Waiting until the process exits doesn’t prevent the warning from firing in the first place. By the time you call process.exit(0), the listeners have already piled up and the warning has already been printed. We need to stop the accumulation before it ever exceeds the limit.


3) How the accumulation happens

Roughly:

watch mode flow
┌─────────────────────────┐    ┌────────────────┐
│ file change detected    │ → │ createServer() │ // registers SIGINT handler
│ → server restarts       │ → │                │ // old handler never removed → leak
└─────────────────────────┘    └────────────────┘

Every server restart stacked another listener, and without a corresponding removeListener the count kept climbing.


4) The fix

  1. Use process.once('SIGINT', …) so the handler only ever registers once.
  2. On server close, explicitly call process.removeListener('SIGINT', …) (and same for SIGTERM) to tear down the handler before the next restart.

This prevents any listener build-up and eliminates the MaxListenersExceededWarning entirely.

Hope that clarifies why we can’t just rely on process.exit() and why manual cleanup is required. Let me know if anything else is unclear!

@BQXBQX

BQXBQX commented Jul 17, 2025

Copy link
Copy Markdown
Collaborator

Well, yes, I reproduced it here, but will there be a scenario in the production environment where the server is repeatedly restarted to monitor the changes of certain files? We are more worried about whether this problem will occur in the formal production environment

What I tested here was that by monitoring the modification of the file and restarting the service repeatedly, I encountered a new problem, which is port conflict. Have you encountered this before?

image
chokidar 'src/**/*.ts' -c 'bun src/index.ts -t sse'

@pchuri

pchuri commented Jul 17, 2025

Copy link
Copy Markdown
Contributor Author

@BQXBQX Great questions! You're absolutely right to think about production implications.

Production Environment Concern

You make a valid point - in most production environments, servers run continuously without frequent restarts, so this specific issue is less common. The main production scenarios where this could occur are:

  • Container orchestration (K8s, Docker Swarm) with rolling updates
  • Process managers that restart on memory limits/crashes
  • Hot-reload enabled staging environments

But honestly, you're right that it's primarily a development/testing issue.

Port Conflict Issue

I haven't personally encountered the port conflict issue you mentioned with chokidar 'src/**/*.ts' -c 'bun src/index.ts -t sse', but here are some approaches that might help:

# Kill previous process first to avoid conflicts
chokidar 'src/**/*.ts' -c 'pkill -f "bun src/index.ts" 2>/dev/null || true; sleep 0.5; bun src/index.ts -t sse'

# Use nodemon which typically handles process cleanup better
nodemon --watch src --ext ts --exec "bun src/index.ts -t sse"

# Or use a different port each time
chokidar 'src/**/*.ts' -c 'PORT=$((RANDOM + 3000)) bun src/index.ts -t sse'

Have you found any specific patterns that trigger the port conflicts?

My Thoughts

Since this is primarily a dev-time improvement and you're concerned about production relevance, I'm totally open to:

  1. Keeping the current approach if you think the cleanup is still worthwhile
  2. Scoping it to development only (e.g., env check)
  3. Closing this PR if you feel it's unnecessary overhead

What's your preference? I want to make sure this aligns with how you envision the project being used.

@BQXBQX

BQXBQX commented Jul 18, 2025

Copy link
Copy Markdown
Collaborator

Thanks for your PR, we will merge this PR

@BQXBQX BQXBQX merged commit 40a33be into hustcc:main Jul 18, 2025
1 check passed
@hustcc

hustcc commented Jul 18, 2025

Copy link
Copy Markdown
Owner
  • mcp-mermaid@0.1.3

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.

4 participants