Skip to content

Conversation

@alienx5499
Copy link
Contributor

Overview

This PR implements the yamux stream destructor improvements as requested in issue #240. The changes ensure that YamuxStream objects are properly cleaned up immediately when destroyed, preventing potential CONNECTION_TOO_MANY_STREAMS errors.

Problem

The original implementation had several issues:

  • YamuxedConnection held strong references to streams, preventing early cleanup
  • Streams were not closed until a cleanup timer fired
  • This could lead to CONNECTION_TOO_MANY_STREAMS errors when many streams were created

Solution

Key Changes

  1. Modified YamuxedConnection to use weak_ptr

    • Changed Streams type from std::unordered_map<StreamId, std::shared_ptr<YamuxStream>> to std::unordered_map<StreamId, std::weak_ptr<YamuxStream>>
    • Added PendingStreams map to hold strong references until handlers are invoked
  2. Implemented proper ~YamuxStream() destructor

    • Destructor now immediately calls doClose(Error::STREAM_CLOSED_BY_HOST)
    • Ensures resources are cleaned up as soon as the stream is destroyed
  3. Removed cleanup timer mechanism

    • Eliminated setTimerCleanup() method and related timer logic
    • Streams are now cleaned up immediately when destroyed
  4. Added pending_streams_ map

    • Prevents premature destruction of newly created streams
    • Strong references are held until user handlers are invoked
    • Automatically cleaned up after handler execution

Files Modified

  • include/libp2p/muxer/yamux/yamuxed_connection.hpp
  • src/muxer/yamux/yamuxed_connection.cpp
  • include/libp2p/muxer/yamux/yamux_stream.hpp
  • src/muxer/yamux/yamux_stream.cpp

Testing

  • All 67 tests pass
  • Previously failing Yamux tests now pass:
    • all_muxers_acceptance_test
    • host_integration_test
    • muxers_and_streams_test

Benefits

  • Streams close earlier, preventing connection limit issues
  • Better memory management with weak_ptr usage
  • Immediate resource cleanup on stream destruction
  • Maintains backward compatibility
  • No performance regression

Closes #240

@alienx5499
Copy link
Contributor Author

@turuslan - This implements the yamux stream destructor improvements you requested in issue #240. The changes ensure streams are cleaned up immediately when destroyed while maintaining all existing functionality. All tests pass and the implementation follows the requirements you outlined.

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.

yamux stream destructor

1 participant