Followup to TCP changes in pr/40817 #40904
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
PR #40904: Size comparison from b034faa to 71e7825 Full report (35 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nxp, psoc6, qpg, realtek, stm32, telink)
|
samadDotDev
reviewed
Sep 10, 2025
gmarcosb
commented
Oct 3, 2025
gmarcosb
commented
Oct 3, 2025
|
PR #40904: Size comparison from c8232e1 to 6af5c44 Full report (3 builds for realtek, stm32)
|
gmarcosb
commented
Oct 3, 2025
|
PR #40904: Size comparison from c8232e1 to f86d48b Full report (30 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, esp32, nxp, qpg, realtek, stm32, telink)
|
gmarcosb
force-pushed
the
tcp-followup
branch
2 times, most recently
from
2f68a81 to
eae6bf3
Compare
gmarcosb
commented
Oct 6, 2025
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #40904 +/- ##
==========================================
+ Coverage 50.94% 50.99% +0.04%
==========================================
Files 1378 1380 +2
Lines 100728 100772 +44
Branches 13065 13057 -8
==========================================
+ Hits 51318 51390 +72
+ Misses 49410 49382 -28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gmarcosb
force-pushed
the
tcp-followup
branch
4 times, most recently
from
05f66d4 to
b060c44
Compare
|
PR #40904: Size comparison from cf7d828 to b060c44 Full report (28 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, esp32, nrfconnect, qpg, realtek, stm32, telink)
|
…s to catch wrong usages of manual ref counting * Use a consistent Free() for all inet connections which does clean-up such as closing the connections before freeing memory back to pool
…ion on the matter thread
…e now has a move operator/ctor)
gmarcosb
force-pushed
the
tcp-followup
branch
from
b63c7e7 to
5f4c8d5
Compare
…yway, and set mAppState
Remove unnecessary initialization as it's now done @ top of method
Fixes CI failure:
==68889==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x000104884514 bp 0x00016b642490 sp 0x00016b642410 T0)
==68889==The signal is caused by a READ memory access.
==68889==Hint: address points to the zero page.
#0 0x104884514 in chip::Transport::TCPBase::FindActiveConnection(chip::Inet::EndPointHandle<chip::Inet::TCPEndPoint> const&) TCP.cpp:205
#1 0x1048826a8 in chip::Transport::TCPBase::HandleAcceptError(chip::Inet::EndPointHandle<chip::Inet::TCPEndPoint> const&, chip::ChipError) TCP.cpp:664
#2 0x10488181c in chip::Transport::TCPBase::HandleIncomingConnection(chip::Inet::EndPointHandle<chip::Inet::TCPEndPoint> const&, chip::Inet::EndPointHandle<chip::Inet::TCPEndPoint> const&, chip::Inet::IPAddress const&, unsigned short) TCP.cpp:623
#3 0x1048b3328 in chip::Inet::TCPEndPointImplSockets::HandleIncomingConnection() TCPEndPointImplSockets.cpp:1066
#4 0x1048b13ac in chip::Inet::TCPEndPointImplSockets::HandlePendingIO(chip::BitFlags<chip::System::SocketEventFlags, unsigned char>) TCPEndPointImplSockets.cpp:797
#5 0x1048a96a8 in chip::Inet::TCPEndPointImplSockets::HandlePendingIO(chip::BitFlags<chip::System::SocketEventFlags, unsigned char>, long) TCPEndPointImplSockets.cpp:783
#6 0x10493f870 in chip::System::LayerImplDispatch::SocketWatch::HandleEvents(chip::System::LayerImplDispatch::SelectSets const&) const SystemLayerImplDispatchSockets.mm:245
#7 0x10493e800 in chip::System::LayerImplDispatch::SocketWatchPool::HandleEvents(chip::System::LayerImplDispatch::SelectSets const&) const SystemLayerImplDispatchSockets.mm:204
#8 0x10493df84 in chip::System::LayerImplDispatch::HandleSocketsAndTimerEvents(std::__1::chrono::duration<unsigned int, std::__1::ratio<1l, 1000l>>) SystemLayerImplDispatchSockets.mm:172
#9 0x104930e28 in chip::System::LayerImplDispatch::HandleDispatchQueueEvents(std::__1::chrono::duration<unsigned int, std::__1::ratio<1l, 1000l>>) SystemLayerImplDispatch.mm:298
gmarcosb
force-pushed
the
tcp-followup
branch
2 times, most recently
from
119a18f to
9c370fc
Compare
gmarcosb
force-pushed
the
tcp-followup
branch
from
9c370fc to
77cfc8f
Compare
|
PR #40904: Size comparison from 5ca0aad to 77cfc8f Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR is a followup to #40817 that refactors TCP endpoint handling to use automatic reference counting with handles. The changes formalize ref-counting by replacing raw pointer usage with handle types and removing direct visibility into endpoint internals, pushing users toward automatic reference counting patterns.
Key changes:
- Replaced
ActiveTCPConnectionHolderwithActiveTCPConnectionHandlethroughout the codebase - Introduced
ReferenceCountedPtras a general-purpose smart pointer for reference counted objects - Changed endpoint types to use handle-based access patterns (
TCPEndPointHandle,UDPEndPointHandle) - Updated endpoint lifecycle management to use automatic cleanup via handles instead of manual Release() calls
Reviewed Changes
Copilot reviewed 59 out of 59 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport/raw/ActiveTCPConnectionState.h | Renamed holder class to handle and switched from AutoRelease to ReferenceCountedPtr base |
| src/transport/raw/TCP.h | Updated method signatures to use handles instead of raw pointers |
| src/transport/raw/TCP.cpp | Implemented handle-based endpoint management and cleanup patterns |
| src/lib/support/ReferenceCountedPtr.h | Added new smart pointer class for reference counted objects |
| src/lib/core/ReferenceCounted.h | Split into ReferenceCountedProtected (private ref-counting) and ReferenceCounted (public) |
| src/inet/TCPEndPoint.h | Changed callback signatures to use handles and made Free() protected |
| src/inet/UDPEndPoint.h | Introduced UDPEndPointHandle and made Free() protected |
| src/inet/InetLayer.h | Updated EndPointManager to work with handles instead of raw pointers |
| src/inet/EndPointBasis.h | Changed base class to ReferenceCountedProtected for controlled access |
| src/system/SystemLayerImplSelect.cpp | Added null pointer check for StopWatchingSocket |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring that introduces ReferenceCountedPtr for managing the lifetime of EndPoint and ActiveTCPConnectionState objects. This moves the codebase towards safer, RAII-based resource management, away from manual Retain/Release calls. The introduction of ReferenceCountedProtected and the ScopeExit utility are also excellent improvements for code safety and clarity. The changes are extensive and mostly look correct and consistent with the goal of the refactoring. I've found one issue that will cause a compilation error.
andy31415
reviewed
Oct 14, 2025
andy31415
reviewed
Oct 14, 2025
bzbarsky-apple
approved these changes
Oct 14, 2025
|
PR #40904: Size comparison from 5ca0aad to 8eaff21 Full report (37 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, nrfconnect, nxp, psoc6, qpg, realtek, stm32, telink)
|
andy31415
reviewed
Oct 14, 2025
andy31415
approved these changes
Oct 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is a followup to #40817 with additional clean-up, formalizing ref-counting & removing visiblity into internals, so that users are pushed to use auto ref counting
Testing
CI