Re: [boost] asio formal review begins

From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2005-12-17 16:57:52

Hi to all,

  I've been playing with asio and here is my review. I've tried to see 
it through the eyes of embedded systems developer, so that we could, for 
example, use asio, in a router or gateway.

-------------------------------------------------------

* Design:

I like the overall design of the library, based on known C++ networking 
patterns. I also like the great use of boost::bind for callback 
functions and the idea of asio::buffer wrapper.

  The flexible error handling is in my opinion, a strong point. Optional 
exceptions should be common in C++ libraries, so that we can use them in 
real-time environments.

  But I think asio covers two aspects: asynchronous programming and 
network. I think both should be separated:

--> boost::asio:    a framework for asynchronous programming
--> boost::net:     a network library.

I think both concepts are mixed now, and don't see how a developer can 
easily plug its asynchronous device (file, network, ipc, or other) in 
asio. Everyone can extend iostream to provide its own classes, but I 
don't see that explanation is asio, I suppose a user should implement 
asio concepts like asynchonous streams but a tutorial on this is a must.

As some have mentioned, I think a separate boost::net library should 
contain synchronous (blocking) sockets that don't need a demuxer to 
work. I find the example of "Timer.1 - Using a timer synchronously" 
disturbing. I think boost::net should provide portable sockets 
(synchronous and asynchronous) that can be used (optionally) in asio 
framework.

If both concepts are separated, boost::filesystem, could for example, be 
used with boost::asio to provide asynchronous file i/o (read, write, 
search, etc...) or this could be used also with inter-process 
communications (which are very similar to network communications)

* Implementation

Good overall. I've following the "Daytime.3 - An asynchronous TCP 
daytime server" example with the Visual 7.1 debugger and specially 
looking for mechanisms that hurt embedded systems. I've found some 
interesting things about memory allocations. I've commented the code 
with the number of calls to "operator new":

int main()
{
   //--> Two allocations constructing a shared_ptr before main enters
   try
   {
     //--> 4 calls to new inside the asio::demuxer constructor
     asio::demuxer demuxer;

     //--> 17 calls to new inside the asio::socket_acceptor constructor
     asio::socket_acceptor acceptor(demuxer,
         asio::ipv4::tcp::endpoint(13));

     // -> 1 explicit new operator
     //--> 4 calls to new inside asio::stream_socket constructor
     asio::stream_socket* socket
       = new asio::stream_socket(demuxer);

     //--> 3 calls to new inside acceptor.async_accept(...)
     // -> 2 in the expression "if(impl == null())" to create
     //    a socket_shared_ptr_type for null()
     // -> 1 to create new operation object.
     acceptor.async_accept(*socket,
         boost::bind(handle_accept, &acceptor, socket,
           asio::placeholders::error));

     //--> 2 calls to new every new operation (accept or completion)
     //   to create a new shared_ptr socket type for new connection
     demuxer.run();
   }
   catch (asio::error& e)
   {
     std::cerr << e << std::endl;
   }

   return 0;
}

void handle_write(asio::stream_socket* socket, char* write_buf,
     const asio::error& /*error*/, size_t /*bytes_transferred*/)
{
   using namespace std; // For free.
   free(write_buf);
   // -> 2 calls to new in the expression "if(impl == null())"
   //    to destroy a socket
   // -> 2 calls to new when assigning impl = null()
   delete socket;
}

void handle_accept(asio::socket_acceptor* acceptor,
     asio::stream_socket* socket, const asio::error& error)
{
   if (!error)
   {
     using namespace std; // For time_t, time, ctime, strdup and strlen.
     time_t now = time(0);
     char* write_buf = strdup(ctime(&now));
     size_t write_length = strlen(write_buf);

     //2 news to create a new shared_ptr send_operation
     asio::async_write(*socket,
         asio::buffer(write_buf, write_length),
         boost::bind(handle_write, socket, write_buf,
           asio::placeholders::error,
           asio::placeholders::bytes_transferred));

     // -> 1 explicit user new operator
     // -> 2 in the expression "if(impl == null())" to create
     //    a socket_shared_ptr_type for null()
     socket = new asio::stream_socket(acceptor->demuxer());

     // -> 2 calls to new in the expression "if(impl == null())"
     //    to create a null socket_shared_ptr_type for null()
     acceptor->async_accept(*socket,
         boost::bind(handle_accept, acceptor, socket,
           asio::placeholders::error));
   }
   else
   {
     delete socket;
   }
}

I find the number of allocations too big. I don't mind if the demuxer or 
the acceptor allocates memory dynamically because those objects have 
long lifetime. The user's explicit allocations are not a problem, 
because we can use a pool if we want to avoid fragmentation.

But I find that repetitive operations (async_accept(), sync_send(), 
run()) make use of dynamic allocations, that I think can be problematic 
in high reliability systems. Some of them come to the fact that 
boost::shared_ptr is used widely in the library. For example, a socket 
is really containing a shared_ptr<unsigned int> and a new created socket 
(for example, when accepting a connection) needs 2 calls to new (one for 
the socket itself and another one for the shared_counter). This way, 
even the null socket is implemented with shared_ptr, so internal 
expressions like:

if(impl == null())

create a temporary null socket that is a fully constructed 
shared_ptr<unsigned int>-> 2 allocations and 2 deallocations. I think 
that copying happily shared_ptr could be expensive because atomic 
operations are cheap but not free due to memory barriers (not maybe in 
asynchronous network operations because those have long latency, but 
maybe in other asynchronous ipc or asynchronous inter-thread 
communications).

    ----------------

It's clear that the use of runtime bind functions as callbacks, every 
asynchronous operation call needs to do a dynamic allocation to store a 
copy of the functor. Asynchronous Completion Tokens like windows' 
OVERLAPPED must be also allocated. asio allocates both in the same 
object and that's good, but there is no way to provide a custom 
allocator, so that the programmer can control and optimize memory use. I 
suggest to avoid implementing sockets as shared_ptr-s and giving the 
user the possibility to provide an allocator to allocate this 
per-asynchronous operation objects.

This maybe is not easy, because each operation (accept, read, write, 
close, etc...) may require a different memory size, but maybe all could 
be unified in a single structure, (the structure would be dependent on
<typename Async_Write_Stream, typename Handler>
but the user knows the types that he uses, so maybe a internal type 
definition

<typename Async_Write_Stream, typename Handler>
struct async_context
{
   typedef /* some_type */ type;
};

could be used so that the user can create a pool of typename 
async_context<my_stream_t, my_handler_t>::type objects and asio could 
use it to allocate objects of that type. Just an idea.

The goal would be that the user could avoid ANY repetitive dynamic 
allocation and build high reliability systems with asio, without fearing 
memory fragmentation issues.

My review has been quite quick so maybe this is not possible because 
there are more subtle problems, but I would want to make asio ready to 
be used from routers to mainframes.

* Documentation

Nice. I think the tutorial is good, but I would add more complex 
examples using more classes. I think that it should contain a chapter 
explaining how asio can be extended to use other asynchronous devices.

* Usefulness

Extremely useful. C++ asynchronous programming and a general network 
implementation is a must.

* Use

I've used asio for some days compiling and debugging some tutorial 
examples in Visual 7.1.

* Me

I've done some asynchronous programming (although not with sockets) and 
I know some of its gotchas. Not an expert but a casual user of 
asynchronous programming

* Summary

I consider asio should be separated in two libraries: _boost::asio_ (as 
asynchronous programming framework) and _boost::net_ (providing socket 
and other network programming portable wrappers). I think that this 
second library should provide synchronous blocking (and non-blocking if 
possible) operations without the use of demuxers or other _boost::asio_ 
classes.

I consider the documentation should contain a chapter explaining how you 
can extend asio to provide new asynchronous devices.

I think the library should review a bit its memory allocation policy, 
and provide to the user the possibility to avoid _any_ repetitive memory 
allocation via customized memory allocation. This will make 
_boost::asio_ extremely portable.

If all conditions are met, I vote STRONG YES to Boost inclusion because 
I think that asio is the best effort around and good basis for a 
Standard C++ network library.

Regards,

Ion