code review please? [boost::asio]

hello all: I was curious if some boost::asio guru would mind checking this out. I just posted my code in, I'd like to know if there's anything that could be done better. Thanks, //client.h #pragma once #include <queue> #include <boost/asio.hpp> #include <boost/bind.hpp> #include "ClientBuffer.h" class Client { protected: boost::asio::ip::tcp::socket _socket; boost::asio::io_service &_service; std::queue<ClientBuffer*> _buffers; bool _writing; void StartWrite(); void HandleWrite(const boost::system::error_code& error); public: Client(boost::asio::io_service &service); ~Client(); virtual boost::asio::ip::tcp::socket& GetSocket(); virtual void Start() = 0; void Write(const char* data); void Write(void* data, unsigned int size); void Write(const std::string &data); }; //client.cpp #include <boost/asio.hpp> #include <boost/bind.hpp> #include "client.h" #include "ClientBuffer.h" Client::Client(boost::asio::io_service &service): _service(service), _socket(service), _writing(false) { } Client::~Client() { } void Client::StartWrite() { //we don't write if we're already writing. We just content ourselves with having pushed the message to the queue. if (_writing) { return; } //we're writing now. _writing = true; ClientBuffer* buffer = NULL; buffer = _buffers.front(); boost::asio::async_write(_socket, boost::asio::buffer(buffer->GetBuffer(), buffer->GetSize()), boost::bind(&Client::HandleWrite, this, boost::asio::placeholders::error)); } void Client::HandleWrite(const boost::system::error_code& error) { if (_buffers.empty()) { _writing = false; return; } ClientBuffer* buffer = _buffers.front(); _buffers.pop(); if (buffer) delete buffer; if (_buffers.empty()) _writing = false; else StartWrite(); } boost::asio::ip::tcp::socket& Client::GetSocket() { return _socket; } void Client::Write(const char* data) { ClientBuffer* buffer = new ClientBuffer(data); _buffers.push(buffer); StartWrite(); } void Client::Write(void* data, unsigned int size) { ClientBuffer* buffer = new ClientBuffer(data, size); _buffers.push(buffer); StartWrite(); } void Client::Write(const std::string &data) { ClientBuffer* buffer = new ClientBuffer(data); _buffers.push(buffer); StartWrite(); } //TelnetClient.h /* *this client is responsible for handling the telnet protocol. */ #pragma once #include <boost/asio.hpp> #include <boost/bind.hpp> #include "client.h" #include "ClientBuffer.h" class TelnetClient:public Client { int _state; boost::asio::streambuf _buffer; public: virtual void Start(); void Read(); void HandleRead(const boost::system::error_code& error); void HandleCommand(const std::string &line); }; //TelnetClient.cpp #include <string> #include <istream> #include <boost/asio.hpp> #include <boost/bind.hpp> #include "client.h" #include "TelnetClient.h" void TelnetClient::Start() { Write("Welcome to SapphireMud.\n"); Write("Use load <player> <password> to load a character.\n"); Write("Use create <player> <password> to create a new player.\n"); _state = 1; Read(); } void TelnetClient::Read() { boost::asio::async_read_until(_socket, _buffer, '\n', boost::bind(&TelnetClient::HandleRead, this, boost::asio::placeholders::error)); } void TelnetClient::HandleRead(const boost::system::error_code& error) { char buffer[2048]; if (error) { //do something here return; } std::istream input(&_buffer); input.getline(buffer, 2048); HandleCommand(buffer); Read(); } void TelnetClient::HandleCommand(const std::string &line) { Write("I don't understand that!\n"); } -- Take care, Ty http://tds-solutions.net The aspen project: a barebones light-weight mud engine: http://code.google.com/p/aspenmud He that will not reason is a bigot; he that cannot reason is a fool; he that dares not reason is a slave.

I was curious if some boost::asio guru would mind checking this out. I just posted my code in, I'd like to know if there's anything that could be done better.
I'm not a guru, but pay attention to 2 points: 1) If _buffers container has several elements, HandleWrite() won't call Write() anymore. In other words, _writing state flag handling is incorrect: the flag should be reset unconditionally in HandleWrite(). 2) The access to _buffers container is not synchronized, so you may call Client::Write() only from the thread that executes io_service::run().

On 11/18/2012 12:13 PM, Igor R wrote:
I was curious if some boost::asio guru would mind checking this out. I just posted my code in, I'd like to know if there's anything that could be done better.
I'm not a guru, but pay attention to 2 points: 1) If _buffers container has several elements, HandleWrite() won't call Write() anymore. In other words, _writing state flag handling is incorrect: the flag should be reset unconditionally in HandleWrite(). 2) The access to _buffers container is not synchronized, so you may call Client::Write() only from the thread that executes io_service::run().
Awesome, thanks. I appreciate both. I'm not totally sure what you mean--I need to lock the thread? My system will have multiple servers running under it. I create services and feed them to a manager which calls Start. This spawns a thread per service. Then I call service.run. Is that going to block the main thread? Is there a way to do what I'm trying--run a thread per server? Thanks,
_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users
-- Take care, Ty http://tds-solutions.net The aspen project: a barebones light-weight mud engine: http://code.google.com/p/aspenmud He that will not reason is a bigot; he that cannot reason is a fool; he that dares not reason is a slave.
participants (2)
-
Igor R
-
Littlefield, Tyler