Replace return True/False with try/except (fixes #194)

[parth-soni07]

What was wrong?

Issue #194

How was it fixed?

• TCP listen() – Raise OpenConnectionError on missing/invalid port or failed start instead of returning False; added tcp_port_str is None check before int() for type safety.
• IListener interface – In abc.py, listen() return type changed from -> bool to -> None; docstring updated to describe raising on failure.
• All IListener implementations – TCP, QUIC, WebSocket, CircuitV2: listen() now returns None and raises on failure; removed return True.
• Mplex stream deadlines – set_deadline(), set_read_deadline(),set_write_deadline() raise ValueError for negative ttl instead of returning False; ABC set_deadline() updated to -> None.
• BasicHost set_stream_handler() – Validate input: raise HostException for empty/whitespace protocol_id or None handler.
• Docstrings – Yamux/QUIC stream set_deadline() and swarm register_notifee() docstrings updated so they no longer claim a False return.
• Tests – TCP: test missing-port raises; mplex: use pytest.raises(ValueError) for invalid ttl; host: test empty protocol_id and None handler; QUIC/websocket: drop success = await listener.listen() / assert success.
• Example – example_mplex_timeouts.py uses try/except ValueError for invalid ttl instead of checking a return value.
• Lint – E501 in tcp.py fixed by shortening the :raises docstring line; test handler made async to satisfy THandler.

Refactors selected APIs to raise exceptions on failure instead of returning True/False, so callers use try/except instead of checking return values.

Summary

• Success: Method returns nothing (implicit None).
• Failure: Method raises an exception; callers use try/except.

Changes

1. Listener listen() (IListener and all implementations)

• Before: async def listen(...) -> bool returned True on success, False on failure.
• After: async def listen(...) -> None; raises (e.g. OpenConnectionError, QUICListenError) on failure.

# Before
success = await listener.listen(maddr, nursery)
assert success

# After
await listener.listen(maddr, nursery)  # raises on failure

Updated: TCP, QUIC, WebSocket, CircuitV2 listeners + ABC IListener in libp2p/abc.py.

2. Mplex stream deadline methods

• Before: set_deadline(), set_read_deadline(), set_write_deadline() returned False for invalid ttl (e.g. negative).
• After: Return None; raise ValueError for invalid ttl.

# Before
if not stream.set_deadline(-1):
    handle_error()

# After
try:
    stream.set_deadline(5.0)
except ValueError:
    handle_error()

Updated: libp2p/stream_muxer/mplex/mplex_stream.py + ABC in libp2p/abc.py.

3. BasicHost set_stream_handler()

• Before: Accepted empty protocol_id or None handler; docstring said "return true if successful".
• After: Validates input; raises HostException for empty/whitespace protocol_id or None handler. No boolean return.

# After (caller)
host.set_stream_handler("/my/protocol", handler)  # raises HostException if invalid

Updated: libp2p/host/basic_host.py.

4. TCP transport listen()

• Before: Returned False on missing/invalid port or failed start.
• After: Raises OpenConnectionError with a clear message. Docstring shortened for line length (E501).

Updated: libp2p/transport/tcp/tcp.py.

5. Docstrings only

• Yamux/QUIC stream set_deadline() and swarm register_notifee(): docstrings updated so they no longer claim a False return.
• Mplex is_closed: docstring clarified.

Tests

• TCP: test_tcp_listener_raises_on_missing_port — asserts OpenConnectionError when port is missing.
• Mplex stream: Tests use pytest.raises(ValueError) for invalid ttl; valid calls expect no exception.
• Host: test_set_stream_handler_raises_on_empty_protocol_id, test_set_stream_handler_raises_on_null_handler.
• Listeners: QUIC/WebSocket tests no longer use success = await listener.listen(...); assert success.

News fragment

See newsfragments/194.bugfix.rst.

[parth-soni07]

CCing @seetadev @acul71 for the initial feedback

[seetadev]

@parth-soni07 : Looks great.

CCing @acul71 and @sumanjeet0012 for their feedback.

We can merge it before the upcoming release on Monday. Please add a newsfragment.

[parth-soni07]

@parth-soni07 : Looks great.

CCing @acul71 and @sumanjeet0012 for their feedback.

We can merge it before the upcoming release on Monday. Please add a newsfragment.

hello @seetadev, I have already pushed a newsfragment in this PR let me know if it needs any changes! Thank You

[acul71]

@parth-soni07 : Looks great.
CCing @acul71 and @sumanjeet0012 for their feedback.
We can merge it before the upcoming release on Monday. Please add a newsfragment.

hello @seetadev, I have already pushed a newsfragment in this PR let me know if it needs any changes! Thank You

Can you please elaborate on this issues:

• Issue: listen() contract now implies transport startup failures should surface as OpenConnectionError, but exceptions raised by nursery.start(serve_tcp, ...) are not wrapped. In failure cases (e.g., bind/start failures), callers may still get raw lower-level exceptions instead of the transport-level error type.
• Issue: IListener.listen docstring names OpenConnectionError, but concrete listeners may raise transport-specific exceptions (QUICListenError, etc.). This can mislead API consumers and documentation readers.

AI PR Review: #1184

1. Summary of Changes

• This PR refactors selected APIs from True/False return signaling to exception-based failure handling, primarily around listener startup and mplex deadline validation.
• Linked issue: #194 ("Change return True/False pattern to try/catch pattern). The issue requests moving away from boolean error signaling in relevant call sites.
• Main affected areas:
  • API interfaces in libp2p/abc.py (IListener.listen, IMuxedStream.set_deadline)
  • Transport listeners in libp2p/transport/tcp/tcp.py, libp2p/transport/quic/listener.py, libp2p/transport/websocket/listener.py, libp2p/relay/circuit_v2/transport.py
  • Stream deadline behavior in libp2p/stream_muxer/mplex/mplex_stream.py
  • Tests and examples updated accordingly
• Breaking/API behavior change: callers can no longer rely on boolean return values for these methods; they now need exception handling semantics.

---

2. Branch Sync Status and Merge Conflicts

Branch Sync Status

• ℹ️ Ahead of origin/main by 8 commits, 0 behind (branch_sync_status.txt: 0 8).

Merge Conflict Analysis

• ✅ No conflicts detected. merge_conflict_check.log reports clean merge test against origin/main.

---

3. Strengths

• Interface and implementation updates are broadly consistent for the bool→exception migration across listeners and mplex deadline methods.
• Test updates correctly move from boolean assertions to exception assertions in changed behavior paths.
• Required newsfragment is present: newsfragments/194.bugfix.rst.
• Maintainer request in PR discussion ("Please add a newsfragment") appears addressed.

---

4. Issues Found

Critical

• None found.

Major

• File: libp2p/transport/tcp/tcp.py
• Line(s): ~110–115 (started_listeners = await nursery.start(...))
• Issue: listen() contract now implies transport startup failures should surface as OpenConnectionError, but exceptions raised by nursery.start(serve_tcp, ...) are not wrapped. In failure cases (e.g., bind/start failures), callers may still get raw lower-level exceptions instead of the transport-level error type.
• Suggestion: Wrap nursery.start(...) in try/except Exception as error and re-raise OpenConnectionError(...) from error to make failure signaling consistent with the API migration and PR intent.
• Suggested patch (diff):

diff --git a/libp2p/transport/tcp/tcp.py b/libp2p/transport/tcp/tcp.py
@@ async def listen(self, maddr: Multiaddr, nursery: trio.Nursery) -> None:
-        started_listeners = await nursery.start(
-            serve_tcp,
-            handler,
-            tcp_port,
-            host_str,
-        )
+        try:
+            started_listeners = await nursery.start(
+                serve_tcp,
+                handler,
+                tcp_port,
+                host_str,
+            )
+        except Exception as error:
+            error_msg = (
+                f"Failed to start TCP listener for {maddr}: {error}"
+            )
+            logger.error(error_msg)
+            raise OpenConnectionError(error_msg) from error

Minor

• File: libp2p/abc.py
• Line(s): ~1428
• Issue: IListener.listen docstring names OpenConnectionError, but concrete listeners may raise transport-specific exceptions (QUICListenError, etc.). This can mislead API consumers and documentation readers.
• Suggestion: Generalize interface docstring to "raises transport-specific listen exception (e.g., OpenConnectionError, QUICListenError)".
• Suggested patch (diff):

diff --git a/libp2p/abc.py b/libp2p/abc.py
@@ class IListener(ABC):
-        Raises
-        ------
-        OpenConnectionError
-            If listening fails (e.g. missing/invalid port or failed start).
+        Raises
+        ------
+        Exception
+            Transport-specific listener exception, such as
+            ``OpenConnectionError`` (TCP/WebSocket) or ``QUICListenError``.

---

5. Security Review

• No new direct security vulnerabilities identified in changed logic.
• Risk surface in this PR is mostly error-propagation semantics rather than input/trust boundary expansion.
• Existing warning in local runs indicates dependency-version mismatch (RequestsDependencyWarning), but this appears environmental/tooling-related rather than introduced by this PR.

---

6. Documentation and Examples

• Example usage was updated in examples/doc-examples/example_mplex_timeouts.py to exception-style behavior.
• Docstrings were updated in several touched modules to align with new semantics.
• Minor remaining clarity issue: interface-level exception doc wording in libp2p/abc.py should be broadened (noted above).

---

7. Newsfragment Requirement

• ✅ Present: newsfragments/194.bugfix.rst
• ✅ Filename format matches required <ISSUE_NUMBER>.<TYPE>.rst
• ✅ PR references an issue (#194) and newsfragment number matches the issue.

---

8. Tests and Validation

Command Results

• make lint: Passed (all checks green).
• make typecheck: Passed (mypy + pyrefly hooks passed).
• make test: Passed (2249 passed, 16 skipped, 24 warnings in 94.60s).
• make linux-docs: Passed (HTML + doctest build succeeded).

Warnings/Observations from Captured Logs

• test_output.log contains RequestsDependencyWarning from requests version compatibility.
• docs_output.log contains the same RequestsDependencyWarning.
• No new failing tests attributable to PR code changes.

Coverage of Changed Behavior

• Positive: tests were updated for changed mplex deadline behavior and listener usage.
• Gap: no focused regression test asserting that TCP listener startup failures (e.g., bind failure/port in use) are surfaced as OpenConnectionError consistently.

---

9. Recommendations for Improvement

• Add a test for TCP listen startup failure path (e.g., conflicting bind) and assert OpenConnectionError.
• Wrap low-level exceptions during TCP startup to preserve the transport-level error contract.
• Clarify IListener.listen interface docstring to match actual cross-transport exception behavior.

---

10. Questions for the Author

• Should IListener.listen formally guarantee a common exception base type for all transports, or is transport-specific exception typing intentional?
• Can we add one explicit TCP startup-failure test to lock in the new exception contract?

---

11. Overall Assessment

• Quality Rating: Good
• Security Impact: Low
• Merge Readiness: Needs fixes
• Confidence: High

[acul71]

Hello @parth-soni07 what's the status of this PR ?

[parth-soni07]

Hello @parth-soni07 what's the status of this PR ?

Hey @acul71, I am having a look at the points you shared in the feedback, will be updating you as soon as I can. Thank you for your patience!

[parth-soni07]

hey @acul71, thanks for the feedback you shared. Here is the summary of the fixes relevant to the feedback.

Issue 1: nursery.start() Exceptions Not Wrapped

Problem:
In libp2p/transport/tcp/tcp.py, the call to await nursery.start(serve_tcp, ...) was not wrapped
in a try/except block. If trio.serve_tcp raised an exception before calling
task_status.started() (e.g. port already in use, bind failure), the raw exception propagated
directly to callers instead of being wrapped as OpenConnectionError.

The existing None check below it was also dead code — nursery.start() never silently returns
None on failure; it either returns the started value or raises.

Fix (libp2p/transport/tcp/tcp.py):
Wrapped the nursery.start() call in a try/except block that catches any exception and re-raises
it as OpenConnectionError. Removed the dead None check.

# Before
started_listeners = await nursery.start(serve_tcp, handler, tcp_port, host_str)
if started_listeners is None:
    raise OpenConnectionError(...)  # never actually reached

# After
try:
    started_listeners = await nursery.start(serve_tcp, handler, tcp_port, host_str)
except Exception as error:
    error_msg = f"Failed to start TCP listener for {maddr}: {error}"
    logger.error(error_msg)
    raise OpenConnectionError(error_msg) from error

---

Issue 2: IListener.listen Docstring Too Specific

Problem:
The Raises section of IListener.listen in libp2p/abc.py named OpenConnectionError
specifically. However, concrete transport implementations raise different exception types
(e.g. QUICListenError in the QUIC listener), making the interface docstring misleading for
API consumers.

Fix (libp2p/abc.py):
Generalized the Raises section to reflect that transport-specific exceptions may be raised.

# Before
Raises
------
OpenConnectionError
    If listening fails (e.g. missing/invalid port or failed start).

# After
Raises
------
Exception
    Transport-specific listener exception, such as
    ``OpenConnectionError`` (TCP/WebSocket) or ``QUICListenError`` (QUIC),
    if listening fails (e.g. missing/invalid port or failed start).

---

Regression Test Added

Why tests missed Issue 1:
All existing listen tests either used port 0 (OS picks a free port, always succeeds) or failed
before nursery.start() was ever reached. No test triggered a bind failure at startup.

Fix (tests/core/transport/test_tcp.py):
Added test_tcp_listener_raises_on_bind_failure which binds a specific port with one listener,
then attempts to bind the same port again and asserts OpenConnectionError is raised — not a
raw OSError from trio.

[acul71]

hey @acul71, thanks for the feedback you shared. Here is the summary of the fixes relevant to the feedback.

Thanks @parth-soni07 for the detailed follow-up and fixes.

I verified the two concerns:

• nursery.start(...) failures are now wrapped as OpenConnectionError.
• IListener.listen docs now correctly describe transport-specific exceptions.

The added bind-failure regression test also looks good. This addresses my earlier feedback from my side.

[acul71]

Hello @seetadev @pacrob
This PR include a fix for windows tox !

##########################

Hi @parth-soni07, thanks for the updates!

Sorry it took me a while to get back to you; I've been buried in PR reviews lately. I added some tests—hope you dig them.

Could you run these examples and share the logs? I’m specifically interested in the listener startup and mplex deadline behavior. We can do this in a follow-up PR if you're able to jump on it right away, just so we don't leave the examples broken., Did a quick check, made some fix, tests looks fine.

Priority examples:

1. examples/doc-examples/example_mplex_timeouts.py
2. examples/echo/echo.py
3. examples/echo/echo_quic.py
4. examples/websocket/websocket_demo.py
5. examples/wss_demo.py
6. examples/transport_integration_demo.py
7. examples/doc-examples/example_quic_transport.py
8. examples/doc-examples/example_running.py
9. examples/circuit_relay/relay_example.py
10. examples/nat/relay.py
11. examples/nat/listener.py
12. examples/identify_push/identify_push_demo.py

Please include:

• exact command used for each example
• whether it passed/failed
• relevant output snippets (or full logs)

Suggested logging format:

source venv/bin/activate
python <example_path>.py ... 2>&1 | tee /tmp/examples/example-<name>.log

[acul71]

LGTM if the examples still runs fine