fix(comms/yamux): dont poll the substream after closing/error

[sdbondi]

Description

fix(comms/yamux): dont poll the substream after closing/error

Motivation and Context

We inadvertently polled the connection after closing it/after an error. This is not correct and results in a panic

Ref libp2p/rust-yamux#199
The PR above prevents the panic and sets the connection state to closed.

This related PR can also be merged before/after this one:
#6909

How Has This Been Tested?

Not explicitly tested

What process can a PR reviewer use to test or verify this change?

Breaking Changes

• None
• Requires data directory on base node to be deleted
• Requires hard fork
• Other - Please specify

Summary by CodeRabbit

• Bug Fixes
  • Improved connection handling to prevent further processing once a connection is closed, enhancing reliability by ensuring that operations halt gracefully when encountering a closed state.
  • Added tracking for the closed state of the worker, enhancing robustness in connection management.

[coderabbitai]

Walkthrough

This PR modifies the YamuxWorker in comms/core/src/multiplexing/yamux.rs by introducing a new boolean field is_closed to track the worker's closed state. The initialization, processing (in the run method), and closing (in the close method) logic have been updated to use this flag, ensuring that once the connection is identified as closed or encounters an error, no further processing is performed. Additionally, the method call style is adjusted to use the instance method (self.close).

Changes

File	Change Summary
comms/core/src/multiplexing/yamux.rs	* Added is_closed: bool field to YamuxWorker. * Updated new, run, and close methods to manage and check the is_closed state. * Replaced Self::close with self.close.

Sequence Diagram(s)

sequenceDiagram
    participant Worker as YamuxWorker
    participant Conn as Connection

    Worker->>Worker: new() initializes is_closed = false
    Worker->>Worker: run() begins processing requests
    alt Error occurs or channel closes
        Worker->>Worker: Set is_closed = true and log message
    else Normal flow
        Worker->>Worker: Continue processing (is_closed remains false)
    end
    Worker->>Worker: close() called with mutable self
    alt Already closed
        Worker-->>Worker: Return early (no action)
    else
        Worker->>Worker: Update state to closed and proceed with cleanup
    end

Loading

Possibly related PRs

• fix: dont poll yamux substream after an error #6909: The changes in the main PR, which introduce a new is_closed field and modify the close method in the YamuxWorker struct, are related to the error handling modifications in the run method of the retrieved PR, as both PRs address the management of connection states and error handling within the same struct.
• feat: add display info to yamux #6904: The changes in the main PR are related to those in the retrieved PR as both modify the YamuxWorker struct and its methods, specifically in how they handle connection information and logging, with the main PR introducing a new field and the retrieved PR enhancing logging with peer connection details.

Poem

I’m a happy rabbit on a coding spree,
Hopping through the changes with glee.
The is_closed flag now leads the way,
Ensuring errors don’t lead us astray.
With clarity in code and a joyful heart,
I celebrate this robust restart!
🐇✨

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5884025 and 0bd7775.

📒 Files selected for processing (1)

• comms/core/src/multiplexing/yamux.rs (6 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (1)

• GitHub Check: ci

🔇 Additional comments (8)

comms/core/src/multiplexing/yamux.rs (8)

246-246: Good addition of the is_closed field.

This boolean field is essential for tracking the connection state and preventing polling after closure, which directly addresses the issue mentioned in the PR objectives.

---

264-264: Correctly initializing is_closed to false.

The field initialization is properly set to false, which makes sense for a newly created worker.

---

281-282: Good change from static to instance method call.

Changing from Self::close to self.close properly leverages the instance method, which allows for tracking the closed state on the instance itself.

---

353-353: Well-placed closure state update on error.

Setting is_closed = true when an error occurs is crucial to prevent further processing of a failed connection. This directly addresses the issue mentioned in the PR objectives about preventing panics.

---

360-368: Good early exit on closed connection.

This check ensures that once a connection is identified as closed, the worker stops processing and logs the state appropriately. This prevents the issue of polling a closed connection, which was the root cause of the panic.

---

373-373: Appropriate method signature update.

Changing handle_request to take a mutable reference to self is necessary to allow this method to interact with the instance's closure state.

---

391-391: Consistent use of instance method.

Using self.close here maintains consistency with the other changes and ensures the closure state is properly tracked on this instance.

---

405-410: Excellent update to the close method.

This implementation:

1. Early returns if already closed, preventing redundant operations
2. Sets the closure state immediately before performing potentially failing operations
3. Properly modifies the instance state via the mutable self reference

This change is central to fixing the issue described in the PR objectives.

✨ Finishing Touches

• 📝 Generate Docstrings

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[github-actions]

Test Results (CI)

    3 files    129 suites   46m 46s ⏱️
1 353 tests 1 353 ✅ 0 💤 0 ❌
4 057 runs  4 057 ✅ 0 💤 0 ❌

Results for commit 0bd7775.

♻️ This comment has been updated with latest results.

[github-actions]

Test Results (Integration tests)

 2 files  11 suites   18m 28s ⏱️
36 tests 35 ✅ 0 💤 1 ❌
37 runs  36 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 0bd7775.

♻️ This comment has been updated with latest results.