Asynchronous leadership election

The following code should be mostly OK, but I'm trying to avoid any stylistic problems or find anything that I overlooked.

The code is an implementation of asynchronous leadership election on a one way ring.

Parts of the implementation are a bit unnatural, because it has some forced features.

main.cpp

```
#include
#include "node.h"
#include
#include
#include
#include
using namespace std;

int main()
{
// initialization
reset_sent_messages();
srand(time(NULL));

// prepare the array
const unsigned total_nodes = 1000;
vector > nodes;
nodes.reserve(total_nodes);

// create nodes, put them into array
for (unsigned i = 0; i (i+1));

// shuffle the nodes randomly
random_shuffle(nodes.begin(),nodes.end());

// connect the nodes
for (unsigned i = 0; i connect(nodes[i+1]);
nodes[total_nodes-1]->connect(nodes[0]);

// prepare the futures to store the elected leader information
vector > leaders;
for (auto i : nodes)
leaders.push_back(i->get_leader());

// run the threads with the main node logic
for (auto i : nodes)
thread([](shared_ptr node) { node->logic(); },i).detach();

/* NOTE:
* For demonstrational purposes we are using promisefuture for final
* synchronization. This is a very unnatural model. Normaly we wouldn't
* detach the threads and use join().
*
* Do note that if a thread isn't detached and the thread variable (returned
* by thread call) is destroyed, the program will be immediately terminated.
* This is due to the fact, that such thread would be effectively leaked.
* Not running in detached mode, but incapable of joining the spawn thread.
*/

// do the final synchronization (so that main doesn't end before the algorithm does)
for (unsigned i = 0; i < total_nodes; i++)
{
if (leaders[i].get() != (int)total_nodes)
throw runtime_error("Node didn't correctly detect it's leader.");
}

// final reports
cout << "Asynchronous

First, a general observation: you have a fair number of typos in your comments. Although the compiler doesn't check such things, I prefer to think of the comments as an integral part of the code, so even minor typos should be fixed.

• Random numbers

Right now, you're using srand() and random_shuffle. Although they were quite new when this question was written, at least if you were doing this today, you'd almost certainly want to use the "new" C++11 random number generators and std::shuffle instead.

• Locking

In a number of places, you have a pattern of locking, carrying out some action, then unlocking, such as:

void Node::receive_message(const pair &message)
{
    // simple blocking implementation
    p_message_buffer_lock.lock();
    p_message_buffer.push(message);
    p_message_buffer_lock.unlock();
}

For such a case, I'd prefer to use std::lock_guard:

void Node::receive_message(const pair &message) { 
    std::lock_guard guard(pmessage_buffer_lock);
    p_message_buffer.push(message);
}

This makes the code a little shorter and simpler (not a big deal, but not a bad thing by any means), but much more importantly it adds quite a bit of exception safety. If (for example) p_message_buffer.push() were to throw an exception, your code wouldn't unlock the lock, but this will.

• Magic numbers

You have magic numbers sprinkled rather liberally throughout the code. For example:

transmit_message(p_id,1);
transmit_message(p_id,2);

Until or unless you've looked at transmit_message, it may not be obvious that these are distances. Even if it's only useful from a documentation viewpoint, I'd at least consider creating a type specifically to represent a transmission distance, and probably give it an explicit constructor, so these would look something like:

transmit_message(p_id, hops(1));
transmit_message(p_id, hops(2));

This seems to me to make the code considerably more self-explanatory, even hops ends up doing essentially nothing other than giving a name/context for the numbers.

• shared_ptr

At least as I read things, the ring of nodes doesn't look like a particularly good use-case for std::shared_ptr. In particular, shared_ptr is intended to represent shared ownership. Using shared_ptr basically asserts that each node in the ring owns its successor node, which doesn't seem very accurate. Equally bad, a ring (or cycle in general) is a case where shared_ptr (or reference counting in general) doesn't work correctly. Each item in the cycle has a reference to the next, so every item always has a non-zero reference count (so they never get cleaned up) even when none of them is accessible any more.

• Comments redux

One other point about comments: right now it seems to me that the comments are really at too low a level to be particularly useful. Just for an obvious example, at some point I'd like to see an explanation of the actual algorithm this is intended to implement. That might well be only a sentence or two like: `send a message around the ring asking who's the leader. Iff the message arrives back at the originating node without any other node claiming leadership, then that node claims leadership."

StackExchange Code Review Q#17736, answer score: 5