boost::asio confusion

[Innominat]

I've been working with the Boost libraries for a few years now, and while they're helpful, boost::asio is a library I find wanting to use, but time and time again, find myself with crashy code. I previously used Qt5's network library which was easy to pick up and I had rather stable code. However, I switched to boost::asio so I could have something more efficient and far less dependency heavy.

I understand the concept of boost::asio, but I seem to be going about coding with it in the wrong way, and looking at examples just poses more questions than answers.

Basically, in psudo code, the guts of my program look like this. If this is too confusing, I can show the real code, as I plan to open source it:

Listen()
{
	User u = new User();
	listener.async_accept(u->socket, [u](error_code ec)
	{
		if (ec) disconnect and log the error;
		else Validate(u);
	});
}

Validate(User *user)
{
	char buf[12]; // 12 byte greeting
	async_read(u->socket, buffer(buf, 12)), [u](error_code ec, size_t)
	{
		if (ec) disconnect and log the error;
		else if (strncmp(buf, "testtesttest", 12) == 0)
		{
			char reply[9] ="testtest";
			async_write(u->socket, buffer(reply, 8), [u](error_code ec, size_t)
			{
				if (ec) disconnect and log the error;
				else
				{
					users.push_back(u);
					ListenForAction();
				}
			});
		}
                else do something with invalid greeting;
	});
}

ListenForAction()
{
	char incoming[16];
	
	async_read(u->socket, buffer(incoming, 16), [u, &incoming](error_code ec, size_t)
	{
		u->mutex.lock();
		if (ec) disconnect, unlock mutex, and log the error;
		else do something with the data that might do more async_read() calls;
	});
}

[Cubbi]

That mutex looks extremely suspicious if, as you say, "more async_read() calls" can take place. If you loaned multiple threads for asio's thread pool. you can use strand to avoid running any of your handlers in parallel.

Other than that, a minimal compilable example would help see what you're missing, possibly. As posted, it doesn't even need async and could be written in just a few lines using asio streams.

[Innominat]

I'm afraid a minimal example would seem quite lengthy. I can omit a bit of it, but it's still a couple hundred lines of code I think. I've heard about strand and how it sends things in order, but I'm not sure how to apply it to async_read/write. Ultimately, I want to be able to have multiple connections do multiple things concurrently (ie. chat while doing a file transfer) hence why I thought mutex locking was needed. Asio streams sounds tempting, but given what my goal is, I think async operations are what I need.

Nevertheless, here's a bit of my current code. I am aware User* would be better off being a shared_ptr. I haven't really touched this code in half a year, but I want to get back to it.

void Server::Listen() // called in ctor
{
	auto u = new User(io);
	
	listener.async_accept(u->sock,
		[this, u](boost::system::error_code ec)
		{
			if (ec)
			{
				Resolve(u);
				Disconnect(u);
				Log(ec.message());
			}
			else
			{
				Resolve(u);
				Log("Incoming connection from " + u->host);
				ValidateHello(u);
			}
			
			Listen();
		});
}

void Server::Resolve(User *u)
{
	tcp::resolver rslv(io); // io is an io_service object in Server
	u->host = rslv.resolve(u->sock.remote_endpoint())->host_name();
}

void Server::ValidateHello(User *u)
{
	using namespace boost::asio;
	
	char *hello = new char[12]; // probably better to have this as std::array or primitive array
	std::fill(hello, hello+12, 0);
	
	async_read(u->sock, buffer(hello, 12),
		[this, u, hello](boost::system::error_code ec, size_t)
		{
			if (ec)
			{
				Log(ec.message());
				Disconnect(u);
			}
			else if (strncmp(hello, "TRTPHOTL\0\1\0\2", 12) != 0)
			{
				Log("["+u->host+"]: Bad connection greeting");
				Disconnect(u);
			}
			else
			{
				char reply[8] = {'T', 'R', 'T', 'P', 0, 0, 0, 0 };
				async_write(u->sock, buffer(reply, 8),
					[this, u](boost::system::error_code ec, size_t)
					{
						if (ec)
						{
							Log(ec.message());;
							Disconnect(u);
						}
						else
						{
							u->id = ++last_user_id;
							users.emplace(u->id, u); // not sure why I have emplace() instead of push_back() here
							ReadTransaction(u);
						}
					});
			}
			
			delete[] hello;
		});
}

void Server::ReadTransaction(User *u)
{
	using namespace boost::asio;
	
	char header[20];
	
	async_read(u->sock, buffer(header, 20),
		[this, u, &header](boost::system::error_code ec, size_t)
		{
			u->lock.lock();
			if (ec)
			{
				if (ec.value() != error::eof) Log(ec.message());
				u->lock.unlock();
				Disconnect(u);
			}
			else
			{
				std::istringstream hss(std::string(header, 20)); // thinking of making this a std::vector<unsigned char> with an io sink
				Transaction *trans = new Transaction(u, hss); // Transaction is a class that holds data to be sent/received and has a 20 byte header
				char *body = new char[trans->size];
				async_read(u->sock, buffer(body, trans->size),
					[this, u, body, trans](boost::system::error_code ec, size_t)
					{
						if (ec)
						{
							Log(ec.message());
							u->lock.unlock();
							Disconnect(u);
						}
						else
						{
							std::istringstream tss(std::string(body, trans->size));
							trans->ReadParams(tss);
							delete[] body;
							
							switch (trans->type)
							{
								case OP_LOGIN: HandleLogin(u, trans); break;
								case OP_AGREED: HandleAgreed(u, trans); break;
								case OP_GETUSERNAMELIST:
									delete trans;
									HandleGetUserNameList(u);
									break;
								case OP_GETCLIENTINFOTEXT: HandleGetUserInfo(u, trans); break;
								case OP_CHATSEND: HandleSendChat(u, trans); break;
								default: u->lock.unlock();
							}
						}
					});
			}
		});
}

void Server::HandleLogin(User *u, Transaction *trans) // this is just one post-read operation I'll put, but there are others
{
	using namespace boost::asio;

	std::ostringstream ss;
	
	if (trans->params.size() > 1)
	{
		u->login = trans->params[0]->AsString();
		std::vector<uint8_t> password = trans->params[1]->AsByteArray();
		SHA256_Init(ctx);
		SHA256_Update(ctx, &password[0], password.size());
		SHA256_Final(u->pw_sum, ctx);
		u->client_ver = trans->params[2]->AsInt16();
	}
	else
	{
		u->login = "guest";
		std::fill(std::begin(u->pw_sum), std::end(u->pw_sum), 0);
		u->client_ver = trans->params[0]->AsInt16();
	}
	delete trans;
	
	trans = new Transaction(u, 0, true, u->last_trans_id, 0);
	trans->params.push_back(new Int16Param(F_USERID, last_user_id));
	trans->params.push_back(new Int16Param(F_VERS, SERVER_VERSION));
	trans->params.push_back(new Int16Param(F_COMMUNITYBANNERID, 0));
	trans->params.push_back(new StringParam(F_SERVERNAME, name.data()));
	trans->Write(ss, true);
	delete trans;
	
	trans = new Transaction(u, OP_SHOWAGREEMENT, false, u->last_trans_id, 0);
	if (agreement.empty())
		trans->params.push_back(new Int16Param(F_NOSERVERAGREEMENT, 1));
	else
		trans->params.push_back(new StringParam(F_DATA, agreement.data()));
	trans->Write(ss, true);
	delete trans;
	
	async_write(u->sock, buffer(ss.str(), ss.str().size()),
		[this, u](boost::system::error_code ec, size_t)
		{
			if (ec)
			{
				Log(ec.message());
				u->lock.unlock();
				Disconnect(u);
			}
			else
			{
				u->lock.unlock();
				ReadTransaction(u);
			}
		});
}

[Cubbi]

a minimal example would seem quite lengthy. I can omit a bit of it, but it's still a couple hundred lines of code I think

"compilable" is an important part of it. Beyond a few lines of code, it doesn't make a lot of sense looking at the issues without a debugger, and that requires a complete program. What *are* the issues, anyway? Are there really no lines of code in 200 that could be removed and still observe them?

I am aware User* would be better off being a shared_ptr

why should it be any kind of pointer? I see there's a container somewhere in the not shown code that registers them, why can't it hold the users directly?

I want to be able to have multiple connections do multiple things concurrently (ie. chat while doing a file transfer) hence why I thought mutex locking was needed

the code that sets up the thread pool is not shown, but even if you give asio just one thread (as the tutorials usually do), it still works concurrently. Just not in parallel.

[Innominat]

Well, running it alongside Wireshark, I've noticed that, often, the transactions are sent in an incorrect order. Even if they're sent correctly, the server tends to segfault under repeated activity, such as multiple logins or multiple chat messages sent. A simple conversation could very well crash the server.