Feat/fix mplex stream muxer issues #742

Aldorax · 2025-07-02T18:08:33Z

PR: Mplex Stream Muxer TODOs Resolution

This PR addresses several TODOs in the Mplex stream muxer implementation, significantly improving type safety, logging, and overall robustness.

What was wrong?

The mplex.py implementation had four outstanding TODO items that detracted from code quality and made debugging difficult:

Return Type Inconsistency: The send_message function was type-hinted to return an int, but its underlying call to write_to_stream returned None. This led to type checker warnings and an unclear API contract.
Missing Logging for Unknown Flags: The system would silently handle and reset streams that received messages with unknown header flags, providing no visibility into this protocol violation. This made diagnosing unexpected behavior challenging.
Missing Warning for Unknown Stream Messages: Messages received for a stream that wasn't recognized by the host were silently dropped, potentially hiding issues with peer behavior or connection state.
Missing Warning for Data After Stream Close: If a remote peer sent data after it had already closed its end of the stream, the data was silently ignored. This could obscure misbehaving peers or protocol errors.

How was it fixed?

This PR resolves all four issues by implementing direct and effective solutions:

Return Type Fixed: The write_to_stream method was updated to return len(_bytes), which represents the number of bytes successfully written. Consequently, the type hints for both write_to_stream and send_message were aligned to correctly return an int, resolving the type inconsistency and removing the # type: ignore comment.
Logging Added: logger.warning calls were strategically added to the handlers for:
- Unknown flags: When messages with unrecognized header flags are received.
- Messages to unknown streams: When data arrives for a stream ID that the local muxer does not recognize.
- Data received after a stream was closed: When a peer attempts to send data on a stream that has already been locally closed.
  This makes protocol violations visible in the logs, which is critical for debugging and understanding peer behavior.
Tests Added: A comprehensive suite of tests was added to validate the new behavior. This includes:
- Runtime type checks for the send_message return value to ensure it consistently returns an int.
- Logging verification tests using the capsys fixture to confirm that the correct warning messages are emitted under the specified error conditions (unknown flags, unknown streams, data after close).

Summary of Approach

The approach was to systematically address each TODO item in the Mplex implementation. The core of the work involved aligning function signatures with their behavior for better type safety and injecting informative logging statements at key points where protocol violations can occur. This significantly enhances the debuggability and robustness of the stream muxer. A corresponding test suite was developed to lock in these fixes and prevent future regressions. The final step was a cleanup pass to ensure all code adhered to the project's linting and static analysis standards.

To-Do

Clean up commit history
Add or update documentation related to these changes
Add entry to the release notes

Cute Animal Picture

The issue this PR #741

…_message when it receives an unknown flag

…rom an unknown stream

… remote after stream was closed by them

…x-stream-muxer-issues

pacrob · 2025-07-10T16:17:04Z

libp2p/abc.py

@@ -2016,39 +2112,6 @@ def is_expired(self) -> bool:
 # ------------------ multiselect_communicator interface.py ------------------


this line should move along with the class definition

Thanks @pacrob Will handle that as I clock in tommorrow.

pacrob · 2025-07-10T16:18:51Z

libp2p/host/basic_host.py

@@ -286,6 +288,25 @@ async def _swarm_stream_handler(self, net_stream: INetStream) -> None:
            )
            await net_stream.reset()
            return
+
+        # --- NEW CODE: Handle case where protocol is None ---


Suggested change

# --- NEW CODE: Handle case where protocol is None ---

# Handle case where protocol is None

pacrob · 2025-07-10T16:19:33Z

libp2p/host/basic_host.py

+            # raise StreamFailure(f"No protocol selected from peer {peer_id}")
+            # But the 'return' is consistent with the `except` block's handling.
+            return
+        # --- END NEW CODE ---


Suggested change

# --- END NEW CODE ---

pacrob · 2025-07-10T16:25:06Z

libp2p/host/basic_host.py

@@ -276,6 +277,7 @@ async def close(self) -> None:
    async def _swarm_stream_handler(self, net_stream: INetStream) -> None:
        # Perform protocol muxing to determine protocol to use
        try:
+            # The protocol returned here can now be None


Suggested change

# The protocol returned here can now be None

pacrob · 2025-07-10T16:31:16Z

libp2p/protocol_muxer/multiselect.py

@@ -48,7 +48,6 @@ def add_handler(
        """
        self.handlers[protocol] = handler

-    # FIXME: Make TProtocol Optional[TProtocol] to keep types consistent
    async def negotiate(
        self,
        communicator: IMultiselectCommunicator,


While you're here, please update the docstring to match the arguments (specifically communicator vs stream).

Sure. Will do.

pacrob · 2025-07-10T16:37:50Z

libp2p/protocol_muxer/multiselect.py

@@ -48,7 +48,6 @@ def add_handler(
        """
        self.handlers[protocol] = handler

-    # FIXME: Make TProtocol Optional[TProtocol] to keep types consistent


You've removed the FIXME comment, but not updated the return type accordingly.

pacrob · 2025-07-10T16:41:17Z

General note on code comments - they should briefly describe what is happening in the following code if it not clear. They should not describe the changes happening in a particular PR. You can add those as comments here on the PR page for things you think need additional clarity.

pacrob · 2025-07-11T21:06:26Z

@Aldorax it looks like this PR duplicates work from #738. Can that one be closed in favor of this one?

seetadev · 2025-07-12T14:28:49Z

@Aldorax it looks like this PR duplicates work from #738. Can that one be closed in favor of this one?

@pacrob : Good find, indeed. Thank you for sharing the feedback to @Aldorax .

@Aldorax : Could you please let us know which PR should be closed out of the 2 PRs? Thanks.

Aldorax · 2025-07-13T13:35:48Z

@Aldorax it looks like this PR duplicates work from #738. Can that one be closed in favor of this one?

@pacrob : Good find, indeed. Thank you for sharing the feedback to @Aldorax .

@Aldorax : Could you please let us know which PR should be closed out of the 2 PRs? Thanks.

@seetadev The other PR can be closed in favor of this one.

Aldorax · 2025-07-13T13:36:26Z

General note on code comments - they should briefly describe what is happening in the following code if it not clear. They should not describe the changes happening in a particular PR. You can add those as comments here on the PR page for things you think need additional clarity.

My bad. Thanks for letting me know. Will follow this comment format going forward.

Aldorax · 2025-07-21T14:53:32Z

@seetadev Changes have been made accordingly

acul71 · 2025-08-08T03:51:11Z

@Aldorax Are you still working on this PR or can be take over ?

Aldorax · 2025-08-08T09:48:08Z

@Aldorax Are you still working on this PR or can be take over ?

I am waiting on a review from @seetadev . I had made changes as request from me in the discussions above. I am still very much active in this.

acul71 · 2025-08-08T11:15:29Z

@Aldorax Are you still working on this PR or can be take over ?

I am waiting on a review from @seetadev . I had made changes as request from me in the discussions above. I am still very much active in this.

Good to hear.
I've seen that some tests are failing, merge last upstream main and go from there.
You can test your PR doing make pr and make docs (or make linux-docs)
Ping me if you need help

manny-uncharted and others added 14 commits July 2, 2025 14:13

REFACTOR: fix arrangement of the classes

7f2c635

Fix(tests): Resolve Muxed Connection Type Assertion Error

ca9bed4

FIX-TESTS: all test cases for test_mux_type compliance passed

8fd2d65

Fix(tests): starting to fix the mplex issue

2fde49e

REFACTOR: added type hinting for send_message and write to stream

9595851

REFACTOR: added a simple warning logging message for _handle_incoming…

f8f93cc

…_message when it receives an unknown flag

FIX: added warning logs on _handle_message when it receives message f…

eb3653e

…rom an unknown stream

FIX: added warning logs on _handle_message when it receives data from…

881e5cb

… remote after stream was closed by them

FIX: added warning logs on _handle_message when it receives data from…

d888938

… remote after stream was closed by them

FIX: added tests and works

80b0f12

FEAT: all test cases passed and works

750991b

Merge branch 'main' of github.com:libp2p/py-libp2p into feat/fix-mple…

5d5e33c

…x-stream-muxer-issues

Merge branch 'main' into feat/fix-mplex-stream-muxer-issues

92b39fe

Merge branch 'main' into feat/fix-mplex-stream-muxer-issues

ec4eb83

pacrob reviewed Jul 10, 2025

View reviewed changes

seetadev mentioned this pull request Jul 13, 2025

Feat/fix return type for mux #738

Closed

3 tasks

Aldorax and others added 2 commits July 21, 2025 15:51

Resolved issues

e0f5dcd

Merge branch 'main' into feat/fix-mplex-stream-muxer-issues

aff17fb

		@@ -2016,39 +2112,6 @@ def is_expired(self) -> bool:
		# ------------------ multiselect_communicator interface.py ------------------

	# --- NEW CODE: Handle case where protocol is None ---
	# Handle case where protocol is None

Feat/fix mplex stream muxer issues #742

Are you sure you want to change the base?

Feat/fix mplex stream muxer issues #742

Uh oh!

Conversation

Aldorax commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR: Mplex Stream Muxer TODOs Resolution

What was wrong?

How was it fixed?

Summary of Approach

To-Do

Cute Animal Picture

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacrob commented Jul 10, 2025

Uh oh!

pacrob commented Jul 11, 2025

Uh oh!

seetadev commented Jul 12, 2025

Uh oh!

Aldorax commented Jul 13, 2025

Uh oh!

Aldorax commented Jul 13, 2025

Uh oh!

Aldorax commented Jul 21, 2025

Uh oh!

acul71 commented Aug 8, 2025

Uh oh!

Aldorax commented Aug 8, 2025

Uh oh!

acul71 commented Aug 8, 2025

Uh oh!

Uh oh!

Aldorax commented Jul 2, 2025 •

edited

Loading