Redesign stoppable objects

[thejohnfreeman]

This change implements a new model for "stoppable" objects, heretofore represented by the Stoppable interface. The benefits include:

• An easier-to-understand intuition
• A simpler prescription for implementations
• Less code that is more readable (around 1000 fewer lines in non-test source)
• Less work and less synchronization at runtime

In this change, stoppable objects are modeled after std::thread, which is already very well documented and widely understood.

std::thread

std::thread is a simple state machine with these states:

1. Empty: A default-constructed std::thread starts in this state. An active std::thread that is joined, detached, or move-assigned to another std::thread transitions to this state.
2. Active: A non-default-constructed std::thread starts in this state. An empty std::thread that is move-assigned from an active std::thread transitions to this state.
3. Destroyed: An empty std::thread whose destructor is called transitions to this state.

An active std::thread that is destroyed or overwritten calls std::terminate, and an empty std::thread that is joined or detached throws an exception, meaning those are not legal transitions. To prevent trying them, callers can check the return value of std::thread::joinable to see which state the object is in: true for active, and false for empty.

Once a std::thread is joined, it never "restarts". Instead, you can move an active std::thread started elsewhere into an empty std::thread.

Methods that initiate a state transition (move assignment, join, and detach) return only after the state transition is complete. State transitions are atomic and synchronous (i.e. blocking).

Stoppable

The new stoppable objects in rippled no longer implement a common interface, but instead follow a common pattern:

std::thread concept	"stoppable" equivalent
empty	stopped
active	started
non-default-constructor	constructor or void start() 1
void join()	void stop()

Unlike Stoppable::onStop and Stoppable::onChildrenStopped, the new stop methods are guaranteed to block until the state transition is complete, just like std::thread::join.

Caveats

I would like to start every stoppable in its constructor, like std::thread, but some stoppable objects are constructed but never started in some tests (e.g. ApplicationImp in TxQ1_test::testMaximum). To accommodate them, we give them a single synchronous start method whose pre-condition is that the object is stopped and whose post-condition is that the object is started.

I would like to give stop the pre-condition that the object is started, matching the behavior of std::thread::join, but some stoppables are "stopped" twice, e.g. LoadManager in the test Subscribe_test::testServer.

I would like to give their destructors the pre-condition that the object is stopped, like std::thread, but some stoppables are never stopped, e.g. JobQueue (which technically "starts" in its constructor) in TxQ1_test::testMaximum.

Stoppables often exhibit overlapping but unnested lifetimes. JobQueue starts in its constructor, called by ApplicationImp's constructor, but ApplicationImp starts in its own start method, or in its setup method, depending on how you look at it, both of which are called after its constructor, but JobQueue is stopped early in the stop method of ApplicationImp. I would prefer that they stop in the reverse order that they started.

RAII

std::thread expects that each non-default-construction matches with exactly one call to std::thread::join. C++ already has an excellent technique for guaranteeing matching function calls: constructors and destructors, and the RAII pattern. std::thread does not follow that pattern, which led frustrated developers to create std::jthread as an alternative. Trying to make the stoppable objects in rippled follow the RAII pattern requires significant changes that I consider just one bridge too far for this changeset. I want to stop here and build support for these changes before pushing further.

Commits

I've tried to make each commit small, separately reviewable, build, and pass all tests, to make it easy to review step-by-step, if that's your preference, but it means there are many commits.

Footnotes

1. Or bool start() in the pesky case of NodeStore::DatabaseShard. ↩

[cjcobb23]

I tested these changes with reporting mode and everything seems to be working fine. Tested both ETL and read-only mode.

[miguelportilla]

👍

[thejohnfreeman]

Please, @cjcobb23 and @cdy20 can you review the merge with 1.8.0-b2? It involves the RelationalDBInterfacePostgres and PgPool and reporting mode conditional compilation.

[cjcobb23]

Left one comment about the merge but I'm good with the code as is.

[cjcobb23]

I don't think you actually need the ifdef here. RelationalDBInterfacePostgres is not conditionally compiled. It's just PgPool that is conditionally compiled (along with the other classes in Pg.h).

[ximinez]

Another incremental review. Nothing really significant.

[ximinez]

I'm finally done going through all of the commits.

[ximinez]

Nice work!