[BOUNTY #8935] Run port check at the beginning of the main.py#13096
[BOUNTY #8935] Run port check at the beginning of the main.py#13096zhaog100 wants to merge 1 commit intoComfy-Org:masterfrom
Conversation
Move port availability check to the beginning of main.py, before custom node loading starts. This prevents users from waiting through slow custom node initialization only to discover the port is taken. Closes Comfy-Org#8935
zhaog100
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
📝 WalkthroughWalkthroughThe change adds an early TCP port availability check during program initialization. When the program starts, it attempts to connect to 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main.py (1)
467-467: Consider usingargs.listeninstead of hardcoded127.0.0.1.The port check connects to
127.0.0.1, but the server binds toargs.listen(line 455). For most cases this works fine since a process bound to0.0.0.0accepts connections on127.0.0.1. However, if someone binds to a specific non-localhost interface (e.g.,--listen 192.168.1.100), a conflict on that interface might not be detected.This is a minor edge case, but for consistency you could check against the first address in
args.listen(or keep127.0.0.1as a reasonable default for the common case).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@main.py` at line 467, The port-check currently uses a hardcoded loopback address when calling test_socket.connect_ex(('127.0.0.1', args.port)), which can miss conflicts when the server actually binds to a specific interface; change the connect_ex target to use the first address from args.listen (or args.listen itself if it's a single address), falling back to '127.0.0.1' if args.listen is empty or set to '0.0.0.0' — update the code that calls test_socket.connect_ex to use that resolved listen address and keep args.port as the port to test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Around line 464-474: The socket created as test_socket can leak if an OSError
occurs before test_socket.close() — modify the block that creates test_socket
(the code that calls socket.socket, test_socket.settimeout, and
test_socket.connect_ex) to ensure the socket is always closed by using a
try/finally or a with/context manager around socket.socket; keep the existing
OSError except branch but move the close into the finally (or rely on the
context manager) so the socket is closed regardless of exceptions and logging
behaviour (logging.error / logging.warning and sys.exit) remains unchanged.
---
Nitpick comments:
In `@main.py`:
- Line 467: The port-check currently uses a hardcoded loopback address when
calling test_socket.connect_ex(('127.0.0.1', args.port)), which can miss
conflicts when the server actually binds to a specific interface; change the
connect_ex target to use the first address from args.listen (or args.listen
itself if it's a single address), falling back to '127.0.0.1' if args.listen is
empty or set to '0.0.0.0' — update the code that calls test_socket.connect_ex to
use that resolved listen address and keep args.port as the port to test.
| try: | ||
| test_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| test_socket.settimeout(1) | ||
| result = test_socket.connect_ex(('127.0.0.1', args.port)) | ||
| test_socket.close() | ||
| if result == 0: | ||
| logging.error("Port {} is already in use. Please specify a different port with --port or stop the process using this port.".format(args.port)) | ||
| sys.exit(1) | ||
| except OSError as e: | ||
| logging.warning("Could not check port {}: {}".format(args.port, e)) | ||
|
|
There was a problem hiding this comment.
Socket may leak if OSError occurs after creation.
If an OSError is raised after the socket is created (line 465) but before close() (line 468), the socket resource leaks. Use a try/finally or context manager to ensure cleanup.
🛠️ Proposed fix using try/finally
try:
test_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
- test_socket.settimeout(1)
- result = test_socket.connect_ex(('127.0.0.1', args.port))
- test_socket.close()
- if result == 0:
- logging.error("Port {} is already in use. Please specify a different port with --port or stop the process using this port.".format(args.port))
- sys.exit(1)
+ try:
+ test_socket.settimeout(1)
+ result = test_socket.connect_ex(('127.0.0.1', args.port))
+ if result == 0:
+ logging.error("Port {} is already in use. Please specify a different port with --port or stop the process using this port.".format(args.port))
+ sys.exit(1)
+ finally:
+ test_socket.close()
except OSError as e:
logging.warning("Could not check port {}: {}".format(args.port, e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@main.py` around lines 464 - 474, The socket created as test_socket can leak
if an OSError occurs before test_socket.close() — modify the block that creates
test_socket (the code that calls socket.socket, test_socket.settimeout, and
test_socket.connect_ex) to ensure the socket is always closed by using a
try/finally or a with/context manager around socket.socket; keep the existing
OSError except branch but move the close into the finally (or rely on the
context manager) so the socket is closed regardless of exceptions and logging
behaviour (logging.error / logging.warning and sys.exit) remains unchanged.
|
👋 Hi! Just a friendly ping on this PR. This is a bounty task submission. Would appreciate your review when you have a moment! 🙏 If there are any changes needed, please let me know. Thanks! 🌶️ |
1 participant
Closes #8935
Summary
Moves port availability check to the very start of
main.py(before custom node loading), so users get an immediate error if the port is already in use instead of waiting through slow custom node initialization.Changes
socket.connect_ex()on127.0.0.1:{args.port}start_comfyui()which loads all custom nodes