feat: Adds peer disconnection message #5755
Conversation
This commit refactors the peer shutdown and failure handling mechanism to be more robust, consistent, and communicative. The previous implementation used raw strings to represent error reasons and did not communicate these reasons to peers when shutting down a connection. With this change disconnections are now explicitly communicated via a `TMClose` protocol message with strongly-typed reasons. This new approach provides better diagnostics and makes the peer disconnection process more stable and predictable.
Tapanito
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #5755 +/- ##
=========================================
- Coverage 78.7% 78.7% -0.0%
=========================================
Files 816 816
Lines 71749 71757 +8
Branches 8475 8479 +4
=========================================
- Hits 56456 56440 -16
- Misses 15293 15317 +24
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| void | ||
| PeerImp::gracefulClose() | ||
| PeerImp::sendAndClose(protocol::TMCloseReason reason) |
There was a problem hiding this comment.
It's not really sendAndClose since it doesn't close the socket. It indicates an intention to close.
| @@ -231,7 +259,8 @@ PeerImp::stop() | |||
| JLOG(journal_.info()) << "Stop"; | |||
| } | |||
| } | |||
| close(); | |||
|
|
|||
| sendAndClose(protocol::TMCloseReason::crSHUTDOWN); | |||
There was a problem hiding this comment.
I think close() should be called here as in the original implementation. The overlay received stop() request. IMO it should close right away.
Also I'm not sure if sendAndClose() would actually close the socket. send() has this code at the start of the function:
auto validator = m->getValidatorKey();
if (validator && !squelch_.expireSquelch(*validator))
{
overlay_.reportOutboundTraffic(
TrafficCount::category::squelch_suppressed,
static_cast<int>(m->getBuffer(compressionEnabled_).size()));
return;
}
If that condition above is true then send() is not going to send anything. And if the send queue is empty then onWriteMessage() is not going to be called and consequently close() is not going to be called.
| JLOG(journal_.info()) << "EOF"; | ||
| return gracefulClose(); | ||
| return close(); |
There was a problem hiding this comment.
I think gracefulClose() should still be called. gracefulClose() calls async_shutdown, which is graceful and secure connection shutdown, especially with SSL/TLS streams. Calling close() might be an ungraceful and often insecure way to terminate a connection.
This PR refactors the peer shutdown and failure handling mechanism to be more robust, consistent, and communicative.
The previous implementation used raw strings to represent error reasons and did not communicate these reasons to peers when shutting down a connection.
With this change disconnections are now explicitly communicated via a
TMCloseprotocol message with strongly-typed reasons. This new approach provides better diagnostics and makes the peer disconnection process more stable and predictable.A similar feature was implemented in #3839 by @gregtatcam, which introduced start
andshutdown` messages. This is a simplified version of that change, introducing only a shutdown message.This feature is backwards compatible, as older
rippledversions ignore unknown p2p protocol messages.Example log message:
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)