Fix the xrpl.net unit test #6241

a1q123456 · 2026-01-20T14:22:21Z

High Level Overview of Change

This PR makes the read function call in handleConnection async, adds a new class TestSink to help debugging, and adds a new target xrpl.tests.helpers to put the helper class in.

Context of Change

Our Trigger job fails almost everyday because of two unit tests. The first issue is that it takes forever for the unit tests to finish, and the second issue is we get a time out error in the xrpl.net test case.

I investigated the second issue and found out that the synchronous function read blocks the connection callback on the client side. As the read function expect some data that is supposed to be sent in the connection callback, we're getting a deadlock here. By making the read function asynchronous, the connection callback will have a chance to be called so that the read function can consume the data eventually.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

codecov · 2026-01-20T14:40:34Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.4%. Comparing base (96d17b7) to head (fa19361).
⚠️ Report is 15 commits behind head on develop.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6241     +/-   ##
=========================================
+ Coverage     79.3%   79.4%   +0.1%     
=========================================
  Files          839     839             
  Lines        71607   71634     +27     
  Branches      8307    8240     -67     
=========================================
+ Hits         56768   56856     +88     
+ Misses       14839   14778     -61

see 13 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/tests/libxrpl/net/HTTPClient.cpp

bthomee · 2026-01-20T16:15:43Z

src/tests/libxrpl/net/HTTPClient.cpp

 {
    // Create a null journal for testing
-    beast::Journal j{beast::Journal::getNullSink()};
+    beast::Journal j{DebugSink::instance()};


Instead of creating a whole new sink, can you use any of the other existing sinks for this? e.g. StreamSink in src/test/unit_test/SuiteJournal.h, TestSink in src/test/server/Server_test.cpp, ...

We can't because those things are in xrpld and this test is a standalone executable. We'll be able to get rid of them completely in the future after we fully switch to gtest.

Co-authored-by: Bart <bthomee@users.noreply.github.com>

src/tests/libxrpl/helpers/DebugSink.cpp

src/tests/libxrpl/helpers/DebugSink.h

src/tests/libxrpl/net/HTTPClient.cpp

Co-authored-by: Bart <bthomee@users.noreply.github.com>

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

src/tests/libxrpl/helpers/TestSink.cpp

bthomee · 2026-01-21T14:07:35Z

src/tests/libxrpl/net/HTTPClient.cpp

+                    if (boost::iequals(name, "Content-Length"))
+                    {
+                        res->erase(boost::beast::http::field::content_length);
+                        res->set(name, value);
+                    }


Suggested change

if (boost::iequals(name, "Content-Length"))

{

res->erase(boost::beast::http::field::content_length);

res->set(name, value);

}

res->set(name, value);

Now that the headers are no longer set before res->body(), shouldn't we set them all here? (I'm not familiar with when Boost sets these fields, so perhaps the erase is still needed if it always adds a Content-Length header by itself.

Note that I'm a bit surprised the tests succeeded without any of the headers being set (unless Boost sets Content-Length automatically).

Then again, this might be because some tests have already been ported to gtest and others haven't, so here you're just lucky it works?

Yeah I think we only have one custom header here and boost always automatically add the content-length header.

Shouldn't we then keep the erase, as otherwise we will get two headers with the "same" name, e.g. Content-Type and content-type?

Something like this might be better:

if (boost::iequals(name, "Content-Length")) { res->erase(boost::beast::http::field::content_length); } res->set(name, value);

Yeah I think it still works without explicitly erasing and re-setting. The logs show that the client is still able to receive different content-length headers.

4: Hello World!" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: content-length: 12 4: 4: Hello World!" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: CONTENT-LENGTH: 12 4: 4: Hello World!" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: Content-length: 12 4: 4: Hello World!" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: content-Length: 12 4: 4: Hello World!" 4: DBG: invokeComplete: Deadline popping: 1 4: [ OK ] HTTPClient.case_insensitive_content_length (10 ms) 4: [ RUN ] HTTPClient.basic_http_request 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: Content-Length: 18 4: Content-Type: text/plain 4: 4: Test response body" 4: DBG: invokeComplete: Deadline popping: 1 4: [ OK ] HTTPClient.basic_http_request (0 ms) 4: [ RUN ] HTTPClient.empty_response 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: Content-Length: 0 4: 4: " 4: DBG: invokeComplete: Deadline popping: 1 4: [ OK ] HTTPClient.empty_response (0 ms) 4: [ RUN ] HTTPClient.different_status_codes 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 200 OK 4: Server: TestServer 4: Content-Length: 10 4: 4: Status 200" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 404 Not Found 4: Server: TestServer 4: Content-Length: 10 4: 4: Status 404" 4: DBG: invokeComplete: Deadline popping: 1 4: TRC: Fetch: 127.0.0.1 4: TRC: Resolving: 127.0.0.1 4: TRC: Resolve complete. 4: TRC: Connected. 4: TRC: Session started. 4: TRC: Wrote. 4: TRC: Header: "HTTP/1.0 500 Internal Server Error 4: Server: TestServer 4: Content-Length: 10 4: 4: Status 500" 4: DBG: invokeComplete: Deadline popping: 1 4: [ OK ] HTTPClient.different_status_codes (1 ms) 4: [----------] 4 tests from HTTPClient (14 ms total) 4: 4: [----------] Global test environment tear-down 4: [==========] 4 tests from 1 test suite ran. (14 ms total) 4: [ PASSED ] 4 tests.

template<class Allocator> void basic_fields<Allocator>:: set_element(element& e) { auto it = set_.lower_bound( e.name_string(), key_compare{}); if(it == set_.end() || ! beast::iequals( e.name_string(), it->name_string())) { set_.insert_before(it, e); list_.push_back(e); return; } for(;;) { auto next = it; ++next; set_.erase(it); list_.erase(list_.iterator_to(*it)); delete_element(*it); it = next; if(it == set_.end() || ! beast::iequals(e.name_string(), it->name_string())) break; } set_.insert_before(it, e); list_.push_back(e); }

The code in boost also erases the existing element

Co-authored-by: Bart <bthomee@users.noreply.github.com>

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

kuznetsss · 2026-01-27T10:39:37Z

src/tests/libxrpl/helpers/TestSink.h

+    static TestSink&
+    instance()
+    {
+        static TestSink _;


_ is an unused variable placeholder in C++26. Maybe I'm thinking too much upfront but probably it would be better to avoid using it for anything important.

src/tests/libxrpl/net/HTTPClient.cpp

kuznetsss · 2026-01-27T10:48:02Z

src/tests/libxrpl/net/HTTPClient.cpp

+            *sock,
+            *buffer,
+            *req,
+            [this, sock, buffer, req](


Move constructor of the class should be explicitly deleted since we are capturing this here to avoid potential problems.

kuznetsss · 2026-01-27T10:54:29Z

src/tests/libxrpl/net/HTTPClient.cpp

+            boost::beast::http::request<boost::beast::http::string_body>>();
+
+        // Read the HTTP request asynchronously
+        boost::beast::http::async_read(


Maybe consider refactoring this to coroutines to avoid a lot of nested lambdas

kuznetsss · 2026-01-27T10:55:45Z

src/tests/libxrpl/net/HTTPClient.cpp

    stop()
    {
        running_ = false;
        acceptor_.close();


You should add some synchronisation here to make sure all the async operations are finished before the class is deleted. E.g. binary_semaphore.acquire() here and binary_semaphore.release() at the end of async operations.

a1q123456 added 2 commits January 20, 2026 12:58

Add a debug sink

a4b6705

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Fix the deadlock issue

cea9894

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

a1q123456 requested review from bthomee and pratikmankawde January 20, 2026 14:22

bthomee reviewed Jan 20, 2026

View reviewed changes

Update src/tests/libxrpl/net/HTTPClient.cpp

b60e8b5

Co-authored-by: Bart <bthomee@users.noreply.github.com>

a1q123456 requested a review from bthomee January 20, 2026 17:05

bthomee reviewed Jan 20, 2026

View reviewed changes

a1q123456 and others added 4 commits January 21, 2026 12:00

Update src/tests/libxrpl/net/HTTPClient.cpp

490512b

Co-authored-by: Bart <bthomee@users.noreply.github.com>

Rename the file

8efea79

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Address PR comments

78818e4

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

Set the header after prepare_payload

710f6f2

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

a1q123456 requested a review from bthomee January 21, 2026 13:38

bthomee reviewed Jan 21, 2026

View reviewed changes

a1q123456 and others added 3 commits January 21, 2026 14:34

Update src/tests/libxrpl/net/HTTPClient.cpp

4f44bb9

Co-authored-by: Bart <bthomee@users.noreply.github.com>

Update src/tests/libxrpl/helpers/TestSink.cpp

dcd9a8b

Co-authored-by: Bart <bthomee@users.noreply.github.com>

Update comments

fa19361

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

bthomee approved these changes Jan 21, 2026

View reviewed changes

bthomee requested a review from kuznetsss January 26, 2026 11:27

a1q123456 added the Needs additional review PR requires at least one more code review approval before it can be merged label Jan 26, 2026

kuznetsss reviewed Jan 27, 2026

View reviewed changes

Fix the xrpl.net unit test #6241

Are you sure you want to change the base?

Fix the xrpl.net unit test #6241

Uh oh!

Conversation

a1q123456 commented Jan 20, 2026 • edited by bthomee Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

Uh oh!

codecov bot commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bthomee Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

a1q123456 commented Jan 20, 2026 •

edited by bthomee

Loading

codecov bot commented Jan 20, 2026 •

edited

Loading

bthomee Jan 21, 2026 •

edited

Loading