Skip to content

Conversation

Minimega12121
Copy link
Contributor

What was wrong?

Issue #
The original send_message call during stream close lacked proper timeout and error handling. This could result in the coroutine hanging indefinitely or raising unhandled exceptions when the underlying connection was unavailable or slow to respond. Additionally, there were TODO comments indicating incomplete error handling logic.

How was it fixed?

Wrapped the send_message call with a trio.fail_after(5) block to enforce a 5-second timeout during stream closure.
Added exception handling for:

  • trio.TooSlowError: Raises a TimeoutError to indicate that stream close took too long.
  • MuxedConnUnavailable: Raises a RuntimeError only if the muxed_conn is not shutting down, preventing silent failures.

These changes ensure proper cleanup logic, avoid indefinite blocking, and improve robustness by making failure modes explicit.

Please take a look @seetadev

@Minimega12121
Copy link
Contributor Author

@seetadev Can you please give me pointers on this PR and how to write it's tests

@seetadev
Copy link
Contributor

@Minimega12121 : Definitely, sharing a discussion page with further details soon.

@seetadev
Copy link
Contributor

@Minimega12121 : HI Archit. Please check #774 . Happy to share pointers on pytest-trio-based test suite that covers the described test cases. Please also review and share feedback, thoughts on #637 . This PR indeed brings an important change.

@Minimega12121
Copy link
Contributor Author

Please review the tests added @seetadev

This PR adds two unit tests to validate the improved error handling and timeout logic in the close() method of the Mplex stream.

test_mplex_stream_close_timeout

  • Purpose: Ensures that the close() method enforces a timeout when send_message hangs.
  • Setup:
    • Mocks send_message to simulate an indefinite hang using trio.sleep_forever().
  • Assertion:
    • Verifies that a TimeoutError is raised after the trio.fail_after(5) timeout.
  • What it Covers: Confirms that the trio.fail_after block correctly prevents the coroutine from hanging indefinitely during stream close.

test_mplex_stream_close_mux_unavailable

  • Purpose: Validates behavior when the underlying muxed_conn is unavailable.
  • Setup:
    • Mocks send_message to raise a MuxedConnUnavailable exception.
  • Assertion:
    • Case 1: If muxed_conn.event_shutting_down is set, stream closes silently (no exception).
    • Case 2: If event_shutting_down is not set, stream close raises a RuntimeError with the appropriate message.
  • What it Covers: Confirms that MuxedConnUnavailable is handled conditionally based on the muxed connection shutdown state.

@seetadev
Copy link
Contributor

@Minimega12121 : Excellent work, Archit. Appreciate your efforts and initiative.

This looks ready for final review + merge.

Congratulations and keep up the great progress :)

@seetadev seetadev merged commit 8352d19 into libp2p:main Jul 26, 2025
28 checks passed
@Minimega12121 Minimega12121 deleted the todo/handletimeout branch July 27, 2025 11:46
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.

2 participants