Skip to content

Commit 2fbda82

Browse files
Apply pre-commit formatting fixes
clang-format and prettier auto-formatting adjustments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 4d9e594 commit 2fbda82

File tree

6 files changed

+53
-60
lines changed

6 files changed

+53
-60
lines changed

BoostToStdCoroutineSwitchPlan.md

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ C++20 stackless coroutines have well-known limitations compared to stackful coro
108108
**Claim**: Stackless coroutines cannot yield from arbitrary stack depths. If `fn_a()` calls `fn_b()` calls `yield()`, only stackful coroutines can suspend the entire chain.
109109

110110
**Analysis**: An exhaustive codebase audit found:
111+
111112
- **1 production yield() call**: `RipplePathFind.cpp:131` — directly in the handler function body
112113
- **All test yield() calls**: directly in `postCoro` lambda bodies (Coroutine_test.cpp, JobQueue_test.cpp)
113114
- **The `push_type*` architecture** makes deep-nested yield() structurally impossible — the `yield_` pointer is only available inside the `postCoro` lambda via the `shared_ptr<Coro>`, and handlers call `context.coro->yield()` at the top level
@@ -119,6 +120,7 @@ C++20 stackless coroutines have well-known limitations compared to stackful coro
119120
**Claim**: Once a function needs to suspend, every caller up the chain must also be a coroutine. This "infects" the call chain.
120121

121122
**Analysis**: In rippled's case, the coloring is minimal:
123+
122124
- `postCoroTask()` launches a coroutine — this is the "root" colored function
123125
- The `postCoro` lambda itself becomes the coroutine function (returns `CoroTask<void>`)
124126
- `doRipplePathFind()` is the only handler that calls `co_await`
@@ -133,6 +135,7 @@ The "coloring" stops at the entry point lambda and the one handler that suspends
133135
**Claim**: C++20 provides the language primitives but no standard task type, executor integration, or composition utilities.
134136

135137
**Analysis**: This is accurate — we need to write custom types:
138+
136139
- `CoroTask<T>` (task/return type) — well-established pattern, ~80 lines
137140
- `JobQueueAwaiter` (executor integration) — ~20 lines
138141
- `FinalAwaiter` (continuation chaining) — ~10 lines
@@ -154,6 +157,7 @@ However, these types are small, well-understood, and have extensive reference im
154157
**Claim**: Coroutine frames are heap-allocated and outlive the calling scope, making references to locals dangerous.
155158

156159
**Analysis**: This is a real concern that requires engineering discipline:
160+
157161
- Coroutine parameters are copied into the frame (safe by default)
158162
- References passed to coroutine functions can dangle if the referent's scope ends before the coroutine completes
159163
- Our design mitigates this: `RPC::Context` is passed by reference but its lifetime is managed by `shared_ptr<Coro>` / the entry point lambda's scope, which outlives the coroutine
@@ -165,6 +169,7 @@ However, these types are small, well-understood, and have extensive reference im
165169
**Claim**: `yield_to.h:111` uses `boost::asio::spawn`, suggesting broader coroutine usage.
166170

167171
**Analysis**: `yield_to.h` uses `boost::asio::spawn` with `boost::context::fixedsize_stack(2 * 1024 * 1024)` — this is a **completely separate** coroutine system:
172+
168173
- Different type: `boost::asio::yield_context` (not `push_type*`)
169174
- Different purpose: test infrastructure for async I/O tests
170175
- Different mechanism: Boost.Asio stackful coroutines (not Boost.Coroutine2)
@@ -175,6 +180,7 @@ However, these types are small, well-understood, and have extensive reference im
175180
#### Overall Viability Conclusion
176181

177182
The migration IS viable because:
183+
178184
1. rippled's coroutine usage is **shallow** (no deep-nested yield)
179185
2. The **colored function infection** is limited to 4 call sites
180186
3. Custom types are **small and well-understood**
@@ -207,6 +213,7 @@ The migration IS viable because:
207213
#### Alternative Considered: ASAN Annotations Only
208214

209215
Instead of full migration, one could keep Boost.Coroutine and add `__sanitizer_start_switch_fiber` / `__sanitizer_finish_switch_fiber` annotations to Coro.ipp to suppress ASAN false positives. This was evaluated and rejected because:
216+
210217
- It only fixes sanitizer false positives — does NOT reduce 1MB/coroutine memory usage
211218
- Does NOT remove the deprecated Boost.Coroutine dependency
212219
- Does NOT provide standard compliance or future-proofing
@@ -400,18 +407,21 @@ Coroutine = std::function<void(Suspend)> — given a Suspend, starts work
400407
```
401408

402409
In practice, `JobQueue::Coro` simplifies this to:
410+
403411
- **Suspend** = `coro->yield()`
404412
- **Continue** = `coro->post()` (async on JobQueue) or `coro->resume()` (sync on current thread)
405413

406414
### 2.6 CMake Dependency
407415

408416
In `cmake/deps/Boost.cmake`:
417+
409418
```cmake
410419
find_package(Boost REQUIRED COMPONENTS ... coroutine ...)
411420
target_link_libraries(xrpl_boost INTERFACE ... Boost::coroutine ...)
412421
```
413422

414423
Additionally in `cmake/XrplInterface.cmake`:
424+
415425
```cpp
416426
BOOST_COROUTINES_NO_DEPRECATION_WARNING // Suppresses Boost.Coroutine deprecation warnings
417427
```
@@ -433,6 +443,7 @@ rippled **already uses C++20 coroutines** in test code:
433443
**Decision: Incremental (multi-phase) migration.**
434444

435445
Rationale:
446+
436447
- Only **one RPC handler** (`RipplePathFind`) actively uses `yield()/post()` suspension
437448
- The **three entry points** (HTTP, WS, gRPC) all funnel through `postCoro()`
438449
- The `RPC::Context.coro` field is the sole propagation mechanism
@@ -689,6 +700,7 @@ sequenceDiagram
689700
### 4.5 RipplePathFind Migration Design
690701

691702
Current pattern:
703+
692704
```cpp
693705
// Continuation callback
694706
auto callback = [&context]() {
@@ -707,6 +719,7 @@ if (request) {
707719
```
708720

709721
Target pattern:
722+
710723
```cpp
711724
// Start async work, suspend via co_await
712725
jvResult = makeLegacyPathRequest(request, /* awaiter-based callback */, ...);
@@ -989,6 +1002,7 @@ gantt
9891002
#### Milestone 1: New Coroutine Primitives (PR #1)
9901003

9911004
**Deliverables**:
1005+
9921006
- `CoroTask<T>` with `promise_type`, `FinalAwaiter`
9931007
- `CoroTask<void>` specialization
9941008
- `JobQueueAwaiter` for scheduling on JobQueue
@@ -997,6 +1011,7 @@ gantt
9971011
- Unit test suite: `CoroTask_test.cpp`
9981012

9991013
**Acceptance Criteria**:
1014+
10001015
- All new unit tests pass
10011016
- Existing `--unittest` suite passes (no regressions from new code)
10021017
- ASAN + TSAN clean on new tests
@@ -1005,32 +1020,37 @@ gantt
10051020
#### Milestone 2: Entry Point Migration (PR #2)
10061021

10071022
**Deliverables**:
1023+
10081024
- `ServerHandler::onRequest()` uses `postCoroTask()`
10091025
- `ServerHandler::onWSMessage()` uses `postCoroTask()`
10101026
- `GRPCServer::CallData::process()` uses `postCoroTask()`
10111027
- `RPC::Context` updated to carry new coroutine type
10121028
- `processSession`/`processRequest` signatures updated
10131029

10141030
**Acceptance Criteria**:
1031+
10151032
- HTTP, WebSocket, and gRPC RPC requests work end-to-end
10161033
- Full `--unittest` suite passes
10171034
- Manual smoke test: `ripple_path_find` via HTTP/WS
10181035

10191036
#### Milestone 3: Handler Migration (PR #3)
10201037

10211038
**Deliverables**:
1039+
10221040
- `RipplePathFind` uses `co_await` instead of `yield()/post()`
10231041
- Path_test and AMMTest updated
10241042
- Coroutine_test and JobQueue_test updated for new API
10251043

10261044
**Acceptance Criteria**:
1045+
10271046
- Path-finding suspension/continuation works correctly
10281047
- All `--unittest` tests pass
10291048
- Shutdown-during-pathfind scenario tested
10301049

10311050
#### Milestone 4: Cleanup & Validation (PR #4)
10321051

10331052
**Deliverables**:
1053+
10341054
- Old `Coro` class and `Coro.ipp` removed
10351055
- `postCoro()` removed from `JobQueue`
10361056
- `Boost::coroutine` removed from CMake
@@ -1039,6 +1059,7 @@ gantt
10391059
- Sanitizer test results documented
10401060

10411061
**Acceptance Criteria**:
1062+
10421063
- Build succeeds without Boost.Coroutine
10431064
- Full `--unittest` suite passes
10441065
- Memory per coroutine ≤ 10KB (down from 1MB)
@@ -1188,13 +1209,15 @@ develop
11881209
```
11891210
11901211
**Workflow**:
1212+
11911213
1. Create sub-branch from `pratik/Switch-to-std-coroutines` for each milestone
11921214
2. Develop and test on the sub-branch
11931215
3. Create PR from sub-branch → `pratik/Switch-to-std-coroutines`
11941216
4. After review + merge, start next milestone sub-branch from the updated feature branch
11951217
5. Final PR from `pratik/Switch-to-std-coroutines` → `develop`
11961218
11971219
**Rules**:
1220+
11981221
- Never push directly to the main feature branch — always via sub-branch PR
11991222
- Each sub-branch must pass `--unittest` and sanitizers before PR
12001223
- Sub-branch names follow the pattern: `pratik/std-coro/<descriptive-action>` (e.g., `add-coroutine-primitives`, `migrate-entry-points`)

include/xrpl/core/CoroTask.h

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,7 @@ class CoroTask<void>
9090
handle_.destroy();
9191
}
9292

93-
CoroTask(CoroTask&& other) noexcept
94-
: handle_(std::exchange(other.handle_, {}))
93+
CoroTask(CoroTask&& other) noexcept : handle_(std::exchange(other.handle_, {}))
9594
{
9695
}
9796

@@ -229,8 +228,7 @@ class CoroTask
229228
handle_.destroy();
230229
}
231230

232-
CoroTask(CoroTask&& other) noexcept
233-
: handle_(std::exchange(other.handle_, {}))
231+
CoroTask(CoroTask&& other) noexcept : handle_(std::exchange(other.handle_, {}))
234232
{
235233
}
236234

include/xrpl/core/JobQueue.h

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,15 +2,14 @@
22

33
#include <xrpl/basics/LocalValue.h>
44
#include <xrpl/core/ClosureCounter.h>
5+
#include <xrpl/core/CoroTask.h>
56
#include <xrpl/core/JobTypeData.h>
67
#include <xrpl/core/JobTypes.h>
78
#include <xrpl/core/detail/Workers.h>
89
#include <xrpl/json/json_value.h>
910

1011
#include <boost/coroutine/all.hpp>
1112

12-
#include <xrpl/core/CoroTask.h>
13-
1413
#include <coroutine>
1514
#include <set>
1615

@@ -146,11 +145,7 @@ class JobQueue : private Workers::Callback
146145
};
147146

148147
// Private: Used in the implementation of postCoroTask
149-
CoroTaskRunner(
150-
create_t,
151-
JobQueue&,
152-
JobType,
153-
std::string const&);
148+
CoroTaskRunner(create_t, JobQueue&, JobType, std::string const&);
154149

155150
// Not copy-constructible or assignable
156151
CoroTaskRunner(CoroTaskRunner const&) = delete;
@@ -512,8 +507,7 @@ template <class F>
512507
std::shared_ptr<JobQueue::CoroTaskRunner>
513508
JobQueue::postCoroTask(JobType t, std::string const& name, F&& f)
514509
{
515-
auto runner = std::make_shared<CoroTaskRunner>(
516-
CoroTaskRunner::create_t{}, *this, t, name);
510+
auto runner = std::make_shared<CoroTaskRunner>(CoroTaskRunner::create_t{}, *this, t, name);
517511
runner->init(std::forward<F>(f));
518512

519513
// Account for the initial suspension (lazy start).

src/test/core/CoroTask_test.cpp

Lines changed: 21 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,7 @@ class CoroTask_test : public beast::unit_test::suite
5555

5656
gate g;
5757
auto runner = env.app().getJobQueue().postCoroTask(
58-
jtCLIENT,
59-
"CoroTaskTest",
60-
[&](auto) -> CoroTask<void> {
58+
jtCLIENT, "CoroTaskTest", [&](auto) -> CoroTask<void> {
6159
g.signal();
6260
co_return;
6361
});
@@ -85,9 +83,7 @@ class CoroTask_test : public beast::unit_test::suite
8583
gate g1, g2;
8684
std::shared_ptr<JobQueue::CoroTaskRunner> r;
8785
auto runner = env.app().getJobQueue().postCoroTask(
88-
jtCLIENT,
89-
"CoroTaskTest",
90-
[&](auto runner) -> CoroTask<void> {
86+
jtCLIENT, "CoroTaskTest", [&](auto runner) -> CoroTask<void> {
9187
r = runner;
9288
g1.signal();
9389
co_await runner->suspend();
@@ -119,9 +115,7 @@ class CoroTask_test : public beast::unit_test::suite
119115

120116
gate g;
121117
env.app().getJobQueue().postCoroTask(
122-
jtCLIENT,
123-
"CoroTaskTest",
124-
[&](auto runner) -> CoroTask<void> {
118+
jtCLIENT, "CoroTaskTest", [&](auto runner) -> CoroTask<void> {
125119
runner->post();
126120
co_await runner->suspend();
127121
g.signal();
@@ -147,9 +141,7 @@ class CoroTask_test : public beast::unit_test::suite
147141
gate g;
148142
int step = 0;
149143
env.app().getJobQueue().postCoroTask(
150-
jtCLIENT,
151-
"CoroTaskTest",
152-
[&](auto runner) -> CoroTask<void> {
144+
jtCLIENT, "CoroTaskTest", [&](auto runner) -> CoroTask<void> {
153145
step = 1;
154146
co_await JobQueueAwaiter{runner};
155147
step = 2;
@@ -193,23 +185,20 @@ class CoroTask_test : public beast::unit_test::suite
193185

194186
for (int i = 0; i < N; ++i)
195187
{
196-
jq.postCoroTask(
197-
jtCLIENT,
198-
"CoroTaskTest",
199-
[&, id = i](auto runner) -> CoroTask<void> {
200-
a[id] = runner;
201-
g.signal();
202-
co_await runner->suspend();
203-
204-
this->BEAST_EXPECT(*lv == -1);
205-
*lv = id;
206-
this->BEAST_EXPECT(*lv == id);
207-
g.signal();
208-
co_await runner->suspend();
209-
210-
this->BEAST_EXPECT(*lv == id);
211-
co_return;
212-
});
188+
jq.postCoroTask(jtCLIENT, "CoroTaskTest", [&, id = i](auto runner) -> CoroTask<void> {
189+
a[id] = runner;
190+
g.signal();
191+
co_await runner->suspend();
192+
193+
this->BEAST_EXPECT(*lv == -1);
194+
*lv = id;
195+
this->BEAST_EXPECT(*lv == id);
196+
g.signal();
197+
co_await runner->suspend();
198+
199+
this->BEAST_EXPECT(*lv == id);
200+
co_return;
201+
});
213202
BEAST_EXPECT(g.wait_for(5s));
214203
a[i]->join();
215204
}
@@ -249,9 +238,7 @@ class CoroTask_test : public beast::unit_test::suite
249238

250239
gate g;
251240
auto runner = env.app().getJobQueue().postCoroTask(
252-
jtCLIENT,
253-
"CoroTaskTest",
254-
[&](auto) -> CoroTask<void> {
241+
jtCLIENT, "CoroTaskTest", [&](auto) -> CoroTask<void> {
255242
g.signal();
256243
throw std::runtime_error("test exception");
257244
co_return;
@@ -282,9 +269,7 @@ class CoroTask_test : public beast::unit_test::suite
282269
int counter = 0;
283270
std::shared_ptr<JobQueue::CoroTaskRunner> r;
284271
auto runner = env.app().getJobQueue().postCoroTask(
285-
jtCLIENT,
286-
"CoroTaskTest",
287-
[&](auto runner) -> CoroTask<void> {
272+
jtCLIENT, "CoroTaskTest", [&](auto runner) -> CoroTask<void> {
288273
r = runner;
289274
++counter;
290275
g.signal();
@@ -332,9 +317,7 @@ class CoroTask_test : public beast::unit_test::suite
332317
env.app().getJobQueue().stop();
333318

334319
auto runner = env.app().getJobQueue().postCoroTask(
335-
jtCLIENT,
336-
"CoroTaskTest",
337-
[&](auto) -> CoroTask<void> { co_return; });
320+
jtCLIENT, "CoroTaskTest", [&](auto) -> CoroTask<void> { co_return; });
338321
BEAST_EXPECT(!runner);
339322
}
340323

src/xrpld/rpc/ServerHandler.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,7 @@ class ServerHandler
169169

170170
private:
171171
Json::Value
172-
processSession(
173-
std::shared_ptr<WSSession> const& session,
174-
Json::Value const& jv);
172+
processSession(std::shared_ptr<WSSession> const& session, Json::Value const& jv);
175173

176174
void
177175
processSession(std::shared_ptr<Session> const&);

src/xrpld/rpc/detail/ServerHandler.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,9 @@
1212
#include <xrpl/basics/base64.h>
1313
#include <xrpl/basics/contract.h>
1414
#include <xrpl/basics/make_SSLContext.h>
15-
#include <xrpl/core/CoroTask.h>
1615
#include <xrpl/beast/net/IPAddressConversion.h>
1716
#include <xrpl/beast/rfc2616.h>
17+
#include <xrpl/core/CoroTask.h>
1818
#include <xrpl/core/JobQueue.h>
1919
#include <xrpl/json/json_reader.h>
2020
#include <xrpl/json/to_string.h>
@@ -376,9 +376,7 @@ logDuration(Json::Value const& request, T const& duration, beast::Journal& journ
376376
}
377377

378378
Json::Value
379-
ServerHandler::processSession(
380-
std::shared_ptr<WSSession> const& session,
381-
Json::Value const& jv)
379+
ServerHandler::processSession(std::shared_ptr<WSSession> const& session, Json::Value const& jv)
382380
{
383381
auto is = std::static_pointer_cast<WSInfoSub>(session->appDefined);
384382
if (is->getConsumer().disconnect(m_journal))
@@ -516,8 +514,7 @@ ServerHandler::processSession(
516514
}
517515

518516
void
519-
ServerHandler::processSession(
520-
std::shared_ptr<Session> const& session)
517+
ServerHandler::processSession(std::shared_ptr<Session> const& session)
521518
{
522519
processRequest(
523520
session->port(),

0 commit comments

Comments
 (0)