[194] Change return True/False pattern to try/catch pattern #923

SuchitraSwain · 2025-09-13T12:23:12Z

PR #923 Review: Input Validation and Error Handling Improvements

Overview

This PR adds comprehensive input validation, exception handling, and test coverage across multiple libp2p modules. While it references Issue #194 (which is already resolved), the actual changes provide significant value for code quality and defensive programming.

Key Changes

✅ Real Value Added

Input Validation: Added checks for empty protocol IDs, null handlers, negative deadlines

Exception Handling: Proper error messages and exception chaining

Test Coverage: 500+ lines of new tests covering edge cases

API Consistency: Unified error handling patterns across modules

Files Modified (25 files)

Core: abc.py, basic_host.py, autonat.py

Stream Muxers: mplex_stream.py, yamux.py

Transport: quic/stream.py, tcp/tcp.py

Tests: Comprehensive test coverage added

Technical Improvements

Before vs After
# Before: No validation, always returned True
def set_deadline(self, ttl: int) -> bool:
    self.read_deadline = ttl
    return True  # Even for negative values!

# After: Proper validation and exceptions
def set_deadline(self, ttl: int) -> None:
    try:
        if ttl < 0:
            raise ValueError("Deadline cannot be negative")
        self.read_deadline = ttl
    except Exception as e:
        raise MuxedStreamError(f"Failed to set deadline: {e}") from e
Test Coverage Added

New Test Files

test_identify_push_exception_handling.py (146 lines) - Exception handling tests

Enhanced test_basic_host.py (151 lines) - Input validation tests

Enhanced test_mplex_stream.py (96 lines) - Deadline validation tests

Test Quality

Input Validation: Tests empty strings, null values, negative numbers

Exception Handling: Tests proper error propagation and chaining

Edge Cases: Comprehensive coverage of failure scenarios

Assessment

✅ Strengths

Defensive Programming: Prevents runtime errors with invalid inputs

Better UX: More descriptive error messages for debugging

Comprehensive Testing: Extensive test coverage for edge cases

Code Quality: Improves maintainability and reliability

⚠️ Issues

Misleading Description: Claims to fix Issue Change return True/False pattern to try/catch pattern #194 (already resolved)

Wrong Premise: The True/False pattern was already eliminated years ago

Recommendation

REFACTOR, DON'T DISCARD

Update PR Description: Remove Issue #194 references, focus on actual improvements:

Input validation and defensive programming

Better error handling and debugging

Comprehensive test coverage

Code quality improvements

Keep the Code: The changes provide real value for:

Preventing runtime errors

Improving debugging experience

Adding defensive programming

Enhancing code reliability

Final Assessment

Status: ✅ Approve with description update

The PR has significant value but needs reframing. The code improvements are excellent - just update the description to focus on the actual benefits rather than the already-resolved issue.

Review based on PR #923 - SuchitraSwain:194-try-catch branch

Cute Animal Picture

- Update multiaddr dependency from 3ea7f866fda9268ee92506edf9d8e975274bf941 to db8124e2321f316d3b7d2733c7df11d6ad9c03e6 - This fixes the build failure caused by missing py-cid branch 'acul71/new-setup' - Resolves Read the Docs build failure in PR libp2p#923

sumanjeet0012

@SuchitraSwain You can take a look at other methods which is still using the True / False return value and change it to responce / exception.

sumanjeet0012 · 2025-09-13T17:06:55Z

libp2p/host/basic_host.py

+        except HostException:
+            raise
+        except Exception as e:
+            raise HostException(f"Failed to set stream handler: {e}") from e


@SuchitraSwain Thank you for adding the try-catch block and input validation to this function. However, I believe this PR may not be addressing the core issue described in #194.
Issue #194 is specifically about "changing return True/False pattern to try/catch pattern," but the True/False pattern has already been removed from py-libp2p in previous commits.
For reference, the earlier implementation at this commit did use the True/False return pattern, but the current codebase has already migrated away from this pattern.

@SuchitraSwain
Change the PR title

pyproject.toml

sumanjeet0012 · 2025-09-14T14:43:43Z

@SuchitraSwain Have you tried running all the tests locally? Are they all passing?
Since these changes may impact other modules, please ensure that all tests pass locally first. After that, we can proceed with running the CI/CD pipeline.

SuchitraSwain · 2025-09-14T16:08:14Z

@SuchitraSwain Have you tried running all the tests locally? Are they all passing? Since these changes may impact other modules, please ensure that all tests pass locally first. After that, we can proceed with running the CI/CD pipeline.

YES I have tested it locally, everything passes

sumanjeet0012 · 2025-09-15T03:47:30Z

@SuchitraSwain resolve the merge conflicts.

sumanjeet0012 · 2025-09-15T03:48:10Z

@seetadev kindly run the CI/CD pipeline.

seetadev · 2025-09-15T05:49:21Z

@sumanjeet0012 : Ran the CI/Cd pipeline. Waiting for test results.

seetadev · 2025-09-15T15:41:28Z

@SuchitraSwain : 2 test cases are failing. Please collaborate with @sumanjeet0012 and @acul71 to get them fixed and arrive at a good conclusion on this PR.

SuchitraSwain · 2025-09-15T19:20:22Z

@seetadev could you please re-run the ci/cd again

SuchitraSwain · 2025-09-15T20:29:38Z

@seetadev The PR is ready

seetadev · 2025-09-18T05:27:24Z

@SuchitraSwain : Thank you so much Suchitra for considering our feedback points. Appreciate it.

Will do a final review today. Please add a test suite and a newsfragment file too.

CCing @sumanjeet0012, who will help you complete these aspects in the PR.

sumanjeet0012

@SuchitraSwain The PR looks good to me and is ready to go from my side. I have just left a minor piece of feedback for your consideration.
Once it is addressed, we will run the CI/CD pipeline and proceed with merging this PR.

tests/core/kad_dht/test_kad_dht.py

seetadev · 2025-09-21T19:50:06Z

Great work team 👏

@SuchitraSwain — thank you for taking the time to carefully rework the return pattern into a proper try/catch exception handling flow, add test coverage, and follow through on the review feedback. This brings much-needed consistency across the codebase and improves clarity for future maintainers.

@sumanjeet0012 — thanks for the thorough review, guidance, and ensuring the changes aligned with the original issue intent. The back-and-forth here really helped refine the PR into a solid contribution.

With the added tests and newsfragment in place, this looks ready to land 🚀

@sumanjeet0012 : Will like you do a final review and add a thumbs up to this comment. Will merge right away.

Appreciate the collaboration from both sides!

seetadev · 2025-09-21T20:36:16Z

@SuchitraSwain : Re-ran the CI/Cd pipeline. Some test cases are failing. Please check and arrive at a good conclusion on the issues.

acul71 · 2025-09-22T00:00:10Z

@SuchitraSwain
I've seen some CI/CD tests failing
ping me if you need help ;-)

SuchitraSwain · 2025-09-22T15:30:07Z

@seetadev please re-ran the pipeline

SuchitraSwain · 2025-09-23T15:36:37Z

Thank you @seetadev

libp2p/stream_muxer/mplex/mplex_stream.py

tests/core/kad_dht/test_kad_dht.py

acul71

PR #923 Review: Input Validation and Error Handling Improvements

Overview

This PR adds comprehensive input validation, exception handling, and test coverage across multiple libp2p modules. While it references Issue #194 (which is already resolved), the actual changes provide significant value for code quality and defensive programming.

Key Changes

✅ Real Value Added

Input Validation: Added checks for empty protocol IDs, null handlers, negative deadlines
Exception Handling: Proper error messages and exception chaining
Test Coverage: 500+ lines of new tests covering edge cases
API Consistency: Unified error handling patterns across modules

Files Modified (25 files)

Core: abc.py, basic_host.py, autonat.py
Stream Muxers: mplex_stream.py, yamux.py
Transport: quic/stream.py, tcp/tcp.py
Tests: Comprehensive test coverage added

Technical Improvements

Before vs After

# Before: No validation, always returned True
def set_deadline(self, ttl: int) -> bool:
    self.read_deadline = ttl
    return True  # Even for negative values!

# After: Proper validation and exceptions
def set_deadline(self, ttl: int) -> None:
    try:
        if ttl < 0:
            raise ValueError("Deadline cannot be negative")
        self.read_deadline = ttl
    except Exception as e:
        raise MuxedStreamError(f"Failed to set deadline: {e}") from e

Test Coverage Added

New Test Files

test_identify_push_exception_handling.py (146 lines) - Exception handling tests
Enhanced test_basic_host.py (151 lines) - Input validation tests
Enhanced test_mplex_stream.py (96 lines) - Deadline validation tests

Test Quality

Input Validation: Tests empty strings, null values, negative numbers
Exception Handling: Tests proper error propagation and chaining
Edge Cases: Comprehensive coverage of failure scenarios

Assessment

✅ Strengths

Defensive Programming: Prevents runtime errors with invalid inputs
Better UX: More descriptive error messages for debugging
Comprehensive Testing: Extensive test coverage for edge cases
Code Quality: Improves maintainability and reliability

⚠️ Issues

Misleading Description: Claims to fix Issue #194 (already resolved)
Wrong Premise: The True/False pattern was already eliminated years ago

Recommendation

REFACTOR, DON'T DISCARD

Update PR Description: Remove Issue #194 references, focus on actual improvements:

Input validation and defensive programming
Better error handling and debugging
Comprehensive test coverage
Code quality improvements

Keep the Code: The changes provide real value for:

Preventing runtime errors
Improving debugging experience
Adding defensive programming
Enhancing code reliability

Final Assessment

Status: ✅ Approve with description update

The PR has significant value but needs reframing. The code improvements are excellent - just update the description to focus on the actual benefits rather than the already-resolved issue.

Review based on PR #923 - SuchitraSwain:194-try-catch branch

seetadev · 2025-09-24T08:31:13Z

@acul71 : Thank you so much for your review and feedback. Appreciate your great support.

@SuchitraSwain , @bomanaps and @yashksaini-coder : Wish if you could address the valuable feedback points shared by @acul71 and make the requisite changes. Once done, kindly ping me. I'll do a final review and then merge.

SuchitraSwain · 2025-09-24T09:08:35Z

I will address the comment by today

seetadev · 2025-09-28T19:56:35Z

@yashksaini-coder , @SuchitraSwain : Re-ran the CI/CD pipeline. We will see the test results soon.

seetadev · 2025-09-29T20:26:49Z

@SuchitraSwain , @yashksaini-coder : This looks good for final review and merge.

Please change the PR description as shared by @acul71

SuchitraSwain · 2025-09-30T12:14:20Z

@seetadev it's done, please merge the PR. Thank you for the great support

seetadev · 2025-09-30T13:05:38Z

@SuchitraSwain : Wish to have @acul71 and @pacrob 's review on this PR. Also, did you mistakenly close the PR request?

SuchitraSwain · 2025-09-30T13:11:05Z

@SuchitraSwain : Wish to have @acul71 and @pacrob 's review on this PR. Also, did you mistakenly close the PR request?

yes it was by mistake.

libp2p/identity/identify_push/identify_push.py

- Fix TCPListener.listen return type to match IListener interface - Fix BasicHost.set_stream_handler boolean context issue - Update demo code to handle new push_identify_to_peer signature - Fix test type errors with TProtocol casting and mock objects - Resolve all linting and formatting issues - All tests now pass with make pr command

acul71 · 2025-09-30T23:28:01Z

Hi @SuchitraSwain . I've been fixing some lint and typecheck errors and fix identify (thanks @pacrob )
Please review.
In some way (change the title or make sure to document what this issue is really about), so to not create confusion about things, like observed by @sumanjeet0012 I believe, some mods where already there but, this PR is anyway an improvement.

seetadev · 2025-10-06T20:22:28Z

@yashksaini-coder and @SuchitraSwain : Thank you for updating the PR. Wish to check if we have covered all the feedback points as shared by @pacrob and me. We will do a final review once we receive a thumbs up from both of you.

CCing @acul71 for his feedback and review too.

SuchitraSwain · 2025-10-07T07:32:21Z

@seetadev yes all the comments are addressed

seetadev · 2025-10-08T13:05:32Z

@seetadev yes all the comments are addressed

@SuchitraSwain : Great to hear. I'll wait to hear for a review from @acul71. Will do a final review + merge after discussion in the maintainer's call.

Suchitra Swain added 2 commits September 13, 2025 14:20

[194] Change return True/False pattern to try/catch pattern

e5f99c6

sumanjeet0012 reviewed Sep 13, 2025

View reviewed changes

comment resolve

33e7c84

SuchitraSwain requested a review from sumanjeet0012 September 14, 2025 14:30

test cases passed

f0c654c

Merge branch 'main' into 194-try-catch

a63fb1b

test fixes

168f287

Merge branch 'main' into 194-try-catch

0a1d158

Merge branch 'main' into 194-try-catch

13448da

test cases added and newsfragment

0ab23fa

sumanjeet0012 suggested changes Sep 19, 2025

View reviewed changes

tests/core/kad_dht/test_kad_dht.py Outdated Show resolved Hide resolved

test cases added and newsfragment

3ad889a

SuchitraSwain requested a review from sumanjeet0012 September 19, 2025 09:41

Merge branch 'main' into 194-try-catch

673329f

test cases fixed

d9f97b6

Merge branch 'main' into 194-try-catch

82d6022

Merge branch 'main' into 194-try-catch

4b04a71

acul71 reviewed Sep 23, 2025

View reviewed changes

libp2p/stream_muxer/mplex/mplex_stream.py Outdated Show resolved Hide resolved

acul71 reviewed Sep 23, 2025

View reviewed changes

tests/core/kad_dht/test_kad_dht.py Outdated Show resolved Hide resolved

acul71 approved these changes Sep 23, 2025

View reviewed changes

Merge branch 'main' into 194-try-catch

91cc29b

yashksaini-coder and others added 3 commits September 24, 2025 19:48

Merge branch 'main' into 194-try-catch

f083be1

comment resolved

74a91d9

Merge branch 'main' into 194-try-catch

412273b

SuchitraSwain closed this Sep 30, 2025

SuchitraSwain reopened this Sep 30, 2025

pacrob reviewed Sep 30, 2025

View reviewed changes

libp2p/identity/identify_push/identify_push.py Show resolved Hide resolved

seetadev and others added 4 commits October 4, 2025 20:28

Merge branch 'main' into 194-try-catch

5c7bd04

Merge branch 'main' into 194-try-catch

b803e27

Merge branch 'main' into 194-try-catch

1d39d08

Merge branch 'main' into 194-try-catch

16f8cf4

yashksaini-coder added 2 commits October 10, 2025 15:44

Merge branch 'main' into 194-try-catch

cc43698

Merge branch 'main' into 194-try-catch

1dc9934

Uh oh!

[194] Change return True/False pattern to try/catch pattern #923

Are you sure you want to change the base?

[194] Change return True/False pattern to try/catch pattern #923

Uh oh!

Conversation

SuchitraSwain commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #923 Review: Input Validation and Error Handling Improvements

Overview

Key Changes

✅ Real Value Added

Files Modified (25 files)

Technical Improvements

Before vs After

Test Coverage Added

New Test Files

Test Quality

Assessment

✅ Strengths

⚠️ Issues

Recommendation

REFACTOR, DON'T DISCARD

Final Assessment

Cute Animal Picture

Uh oh!

sumanjeet0012 left a comment

Choose a reason for hiding this comment

Uh oh!

sumanjeet0012 Sep 13, 2025

Choose a reason for hiding this comment

Uh oh!

acul71 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sumanjeet0012 commented Sep 14, 2025

Uh oh!

SuchitraSwain commented Sep 14, 2025

Uh oh!

sumanjeet0012 commented Sep 15, 2025

Uh oh!

sumanjeet0012 commented Sep 15, 2025

Uh oh!

seetadev commented Sep 15, 2025

Uh oh!

seetadev commented Sep 15, 2025

Uh oh!

SuchitraSwain commented Sep 15, 2025

Uh oh!

SuchitraSwain commented Sep 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seetadev commented Sep 18, 2025

Uh oh!

sumanjeet0012 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

seetadev commented Sep 21, 2025

Uh oh!

seetadev commented Sep 21, 2025

Uh oh!

acul71 commented Sep 22, 2025

Uh oh!

SuchitraSwain commented Sep 22, 2025

Uh oh!

SuchitraSwain commented Sep 23, 2025

Uh oh!

Uh oh!

Uh oh!

acul71 left a comment

Choose a reason for hiding this comment

PR #923 Review: Input Validation and Error Handling Improvements

Overview

Key Changes

✅ Real Value Added

Files Modified (25 files)

Technical Improvements

Before vs After

SuchitraSwain commented Sep 13, 2025 •

edited

Loading

SuchitraSwain commented Sep 15, 2025 •

edited

Loading

acul71 commented Sep 30, 2025 •

edited

Loading