Added timeout for handling read and write operations

[parth-soni07]

What was wrong?

Issue #723

How was it fixed?

Modified the read() and write() methods to actually check and enforce the deadlines.

[paschal533]

Hi @parth-soni07 , thanks for the quick fixes. The code looks great now. You've resolved the duplicate receive issue and the EOF reading is now using the timeout version like it should.

I also noticed you moved the ReadWriteLock to its own module which is a nice improvement for code organization.

The deadline handling is now working properly throughout the read and write operations, and the error messages are clear and helpful. This is good to merge. Nice work getting the deadline functionality working, this has been a TODO for a while. CC @seetadev and @pacrob

[parth-soni07]

Hi @parth-soni07 , thanks for the quick fixes. The code looks great now. You've resolved the duplicate receive issue and the EOF reading is now using the timeout version like it should.

I also noticed you moved the ReadWriteLock to its own module which is a nice improvement for code organization.

The deadline handling is now working properly throughout the read and write operations, and the error messages are clear and helpful. This is good to merge. Nice work getting the deadline functionality working, this has been a TODO for a while. CC @seetadev and @pacrob

Hey @paschal533 , thank you for your review! :)

[seetadev]

@parth-soni07 : Appreciate your efforts and contribution. Doing a detailed review. Will share feedback soon.

Also, CCing @acul71 and @Jineshbansal, who were working on this feature earlier.

Wish if you could resolve the merge conflicts. Once resolved, please ping me.

I will re-run the CI/CD pipeline and share test results soon.

[pacrob]

Good work, @parth-soni07! This still needs a newsfragment and testing to verify things are working as expected and to prevent regression.

[acul71]

@parth-soni07
Is this merged PR related ?
#987

Implement MplexStream read and write deadline timeout enforcement #987
Merged
acul71 merged 3 commits into libp2p:main from bomanaps:fix/mplexstream yesterday

[Jineshbansal]

You may use trio.current_time() instead of time.time().
time.time() returns wall-clock time, which can jump backward or forward if the system clock changes (e.g., due to NTP).
trio.current_time() is monotonic and stable, making it the safer choice for deadlines and timeouts in async code.

[acul71]

Hello @parth-soni07 , thanks, for this PR.
The merged PR #987 already fix the issue.
Your PR try to fix the issue with a slightly more accurate approach.
Hey @seetadev I'm undecided if we should close this, or dig into it further.
What's your opinion?

Summary

PR #995 should be closed because:

1. PR Implement MplexStream read and write deadline timeout enforcement #987 already fixed the issue

   • Discussion # TODO Analysis: MplexStream Issues #723 noted that deadline enforcement was missing
   • PR Implement MplexStream read and write deadline timeout enforcement #987 (merged 2025-10-19) implemented deadline timeout enforcement
   • The original problem is resolved

2. PR Added timeout for handling read and write operations #995 introduces bugs without clear benefits

   • Type errors (passing float to function expecting int)
   • Removed validation (negative TTL values now accepted)
   • 10 test failures due to breaking changes
   • No documented justification for the refactoring

3. No clear justification

   • No issue explaining why absolute timestamps are needed
   • No documented problems with PR Implement MplexStream read and write deadline timeout enforcement #987's TTL approach
   • Minimal (if any) benefits over the current implementation

Comparison

Aspect	PR #987 (Current)	PR #995 (Proposed)
Status	✅ Working, tested, merged	❌ Broken, has bugs
Type Safety	✅ Passes	❌ Type errors
Tests	✅ All pass	❌ 10 failures
Validation	✅ Validates input	❌ No validation
Justification	✅ Fixed issue #984	❌ No clear reason

Recommendation

Close PR #995 with a comment explaining:

• PR Implement MplexStream read and write deadline timeout enforcement #987 already fixed the deadline enforcement issue
• The refactoring lacks justification and introduces bugs
• If the author wants to pursue this, they should:
  1. Open a new issue explaining why absolute timestamps are needed
  2. Fix all bugs (type errors, validation, tests)
  3. Provide clear benefits over the current TTL approach

Bottom line: PR #995 is a refactoring attempt that introduces bugs without clear benefits. The original issue is already fixed, so this PR is not needed.

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

PR #995 Relevance Analysis: Should This PR Be Closed?

Executive Summary

Recommendation: PR #995 should be CLOSED unless the author can provide clear justification for the refactoring and fix all critical bugs.

Reasoning:

• PR Implement MplexStream read and write deadline timeout enforcement #987 already fixed the deadline enforcement issue from discussion # TODO Analysis: MplexStream Issues #723
• PR Added timeout for handling read and write operations #995 introduces bugs and breaking changes without clear benefits
• The refactoring from TTL to absolute timestamps lacks documented justification
• Multiple critical blockers prevent merge (type errors, test failures, missing validation)

---

Comparison: PR #987 vs PR #995

PR #987 (Merged 2025-10-19) - Current Implementation

Approach: TTL-based deadline enforcement

• Stores deadlines as int | None (TTL in seconds)
• Uses _with_timeout() wrapper that takes TTL directly
• Validates negative TTL values (returns False)
• Works correctly - all tests pass
• Simple and straightforward implementation

Code Example:

read_deadline: int | None  # TTL in seconds

def set_read_deadline(self, ttl: int) -> bool:
    if not self._validate_ttl(ttl):
        return False
    self.read_deadline = ttl
    return True

async def read(self, n: int | None = None) -> bytes:
    return await self._with_timeout(
        self.read_deadline, "Read", lambda: self._do_read(n)
    )

Status: ✅ Working, tested, merged

---

PR #995 (Current) - Proposed Changes

Approach: Absolute timestamp-based deadline enforcement

• Stores deadlines as float | None (absolute timestamps)
• Converts TTL to absolute timestamp: time.time() + ttl
• Adds deadline checking at multiple points (before lock, after lock, during operations)
• Removes validation for negative TTL values
• Has type errors (passing float to _with_timeout() which expects int)
• Breaks 10 tests due to format change

Code Example:

read_deadline: float | None  # Absolute timestamp

def set_read_deadline(self, ttl: int) -> bool:
    self.read_deadline = time.time() + ttl  # No validation!
    return True

async def read(self, n: int | None = None) -> bytes:
    self._check_read_deadline()  # Check before starting
    # ... more deadline checks ...
    return await self._with_timeout(
        self.read_deadline, "Read", lambda: self._do_read(n)
        # ❌ TYPE ERROR: read_deadline is float, _with_timeout expects int
    )

Status: ❌ Broken, has bugs, tests fail

---

Key Differences

Aspect	PR #987 (Current)	PR #995 (Proposed)
Deadline Format	TTL (int) - seconds from now	Absolute timestamp (float)
Validation	✅ Validates negative TTL	❌ No validation
Type Safety	✅ Type checks pass	❌ Type errors
Test Status	✅ All tests pass	❌ 10 tests fail
Complexity	Simple wrapper approach	Multiple checkpoints
Accuracy	Accurate (TTL used immediately)	More accurate (accounts for elapsed time)
Breaking Changes	None	Yes (format change)

---

Potential Benefits of PR #995's Approach

1. More Accurate Timeout Calculation

   • Absolute timestamps account for time elapsed between setting deadline and checking
   • TTL approach is still accurate because _with_timeout() is called immediately

2. Multiple Deadline Checkpoints

   • PR Added timeout for handling read and write operations #995 checks deadlines before lock acquisition, after lock acquisition, and during operations
   • PR Implement MplexStream read and write deadline timeout enforcement #987 only checks at operation start (via _with_timeout())
   • However: Lock acquisition is typically fast, so this may not provide significant benefit

3. Early Deadline Detection

   • Can detect if deadline already expired before starting operation
   • However: PR Implement MplexStream read and write deadline timeout enforcement #987's _with_timeout() also handles this correctly

---

Problems with PR #995

Critical Issues

1. Type Errors (BLOCKER)

   • _with_timeout() expects int | None but receives float | None
   • Prevents code from passing type checking
   • Fix requires either:
     • Removing _with_timeout() calls (since _do_read()/_do_write() handle timeouts)
     • Updating _with_timeout() signature
     • Converting absolute timestamp back to TTL before calling

2. Removed Validation (BLOCKER)

   • Negative TTL values are now silently accepted
   • Results in deadlines in the past (immediate timeout)
   • Breaks expected behavior and tests

3. Breaking Changes (BLOCKER)

   • Changes deadline format from int to float
   • Breaks 10 tests that expect TTL format
   • No migration path or documentation

4. Missing Issue Reference (BLOCKER)

   • References discussion # TODO Analysis: MplexStream Issues #723 (not an issue)
   • No valid issue explaining why refactoring is needed

5. Missing Newsfragment (BLOCKER)

   • Required for PR approval
   • No newsfragment file exists

Test Failures

10 tests fail because they expect TTL format:

• test_mplex_stream_set_write_deadline_only - expects write_deadline == 30, gets timestamp
• test_mplex_stream_deadline_update - expects read_deadline == 10, gets timestamp
• test_mplex_stream_deadline_independent - expects read_deadline == 10, gets timestamp
• test_mplex_stream_deadline_validation_negative_ttl - expects validation, gets none
• test_mplex_stream_deadline_validation_zero_ttl - expects read_deadline == 0, gets timestamp
• test_mplex_stream_deadline_validation_positive_ttl - expects read_deadline == 1, gets timestamp
• test_mplex_stream_write_deadline_timeout - expects TimeoutError, gets MplexStreamTimeout
• test_mplex_stream_set_deadline_sets_both - expects read_deadline == 30, gets timestamp
• test_mplex_stream_set_read_deadline_only - expects read_deadline == 30, gets timestamp
• test_mplex_stream_read_deadline_timeout - exception type mismatch

---

Is PR #995 Still Relevant?

Arguments for Keeping PR #995

1. Potential Accuracy Improvement

   • Absolute timestamps might be more accurate for long-running operations
   • However, PR Implement MplexStream read and write deadline timeout enforcement #987's TTL approach is accurate because timeout is applied immediately

2. Multiple Checkpoints

   • More thorough deadline checking
   • However, lock acquisition is typically fast, so benefit is minimal

3. Better Error Messages

   • Custom MplexStreamTimeout exception
   • However, PR Implement MplexStream read and write deadline timeout enforcement #987 already has good error messages via TimeoutError

Arguments for Closing PR #995

1. PR Implement MplexStream read and write deadline timeout enforcement #987 Already Fixed the Issue

   • Discussion # TODO Analysis: MplexStream Issues #723's concern (deadline not enforced) is resolved
   • PR Implement MplexStream read and write deadline timeout enforcement #987's implementation works correctly

2. No Clear Justification

   • No documented problems with PR Implement MplexStream read and write deadline timeout enforcement #987's approach
   • No issue explaining why refactoring is needed
   • No performance or correctness benefits demonstrated

3. Introduces Bugs

   • Type errors prevent merge
   • Removed validation breaks expected behavior
   • Breaking changes break tests

4. High Risk, Low Reward

   • Significant refactoring with breaking changes
   • Minimal (if any) benefits over current implementation
   • Requires updating all tests and potentially breaking user code

---

Recommendation

Option 1: Close PR #995 (Recommended)

Reason: PR #987 already fixed the issue. PR #995 is a refactoring attempt that:

• Lacks clear justification
• Introduces bugs and breaking changes
• Has multiple blockers preventing merge
• Provides minimal (if any) benefits over current implementation

Action: Close PR with comment explaining that:

• PR Implement MplexStream read and write deadline timeout enforcement #987 already fixed the deadline enforcement issue
• The refactoring lacks justification and introduces bugs
• If the author wants to pursue this refactoring, they should:
  1. Open a new issue explaining why absolute timestamps are needed
  2. Fix all bugs (type errors, validation, tests)
  3. Provide clear benefits over current TTL approach

Option 2: Request Major Changes (If Author Wants to Continue)

Reason: If there's a valid reason for absolute timestamps, the PR needs significant work.

Required Changes:

1. Open a new issue explaining the refactoring motivation
2. Fix type errors (remove _with_timeout() calls or update signature)
3. Restore TTL validation
4. Update all tests to expect absolute timestamps
5. Add newsfragment
6. Document breaking changes
7. Provide clear justification for the refactoring

Likelihood of Success: Low - requires significant work and justification

---

Conclusion

PR #995 should be CLOSED because:

1. ✅ The original issue (deadline enforcement) is already fixed by PR Implement MplexStream read and write deadline timeout enforcement #987
2. ❌ PR Added timeout for handling read and write operations #995 introduces bugs without clear benefits
3. ❌ Multiple critical blockers prevent merge
4. ❌ No documented justification for the refactoring
5. ❌ Breaking changes with no migration path

If the author believes absolute timestamps provide significant benefits, they should:

• Open a new issue documenting the problems with TTL approach
• Fix all bugs in a new PR
• Provide clear justification and benefits

Current Status: PR #995 is not ready for merge and lacks clear justification for the refactoring.