[boost] asio review

From: Dirk Moermans (dirkmoermans_at_[hidden])
Date: 2005-12-20 16:27:24

Hi,

I vote to accept asio into boost. It will still require a lot of work
and is by no means finished. My impression is that most issues can be
solved incrementally though. The supported features are sufficient
(number of platforms + IPV4) to prove that the concepts will work.

The documentation is insufficient to understand the interface. I had
to read the implementation and the examples. I have spent 8 hours on
the review (reading documentation and reading code, I have not written
any code using asio). I have experience with ACE, MFC and  socket.h
(mostly synchronous though, so asio has been an eye-opener for me).
This is my first boost-review.

-- design:

I like the acceptor-connector pattern as used in ACE. It seems that
you do use an acceptor class, but did not implement connector classes.
It would be nice to have separate classes for connection-establishment
and for connection-handling, for both client- and server-side
(separation of concerns).

I dislike the use of the "services pattern". It might make sense to
put the service classes in the details section but they do not belong
in the public interface. Such a design would not be appropriate for
standardization. I wonder if you can reach the same goals by using
concept-checks and some socket_traits classes instead of duplicating
every interface. I did read the description in the design section and
I am still not convinced that they are a good solution for the stated
problems.

I think the following code-snippet in the tutorial could be written
more elegantly, through an interface change of the deadline_timer:

{
boost::asio::deadline_timer t;
...
 t.expires_at(t.expires_at() + boost::posix_time::seconds(1));
}

by giving direct access to the time member.

t.expiry_date()+=boost::posix_time::seconds(1);
For more complex cases or error-checking, some proxy classes could be used.

You might want to add swap member-functions implemented in  e.g. the
address class.

You could overload more functions that take a const std::string&
parameter with a const char* parameter (e.g. the host constructor).

You seem to return a std::string in the some member functions of host
and address classes, where you could return a const std::string&. e.g.

  in host.hpp
  std::string host::name() const

 -- implementation

Why do you catch exceptions by reference instead of const reference :-) ?

Why do you roll out your own threads-implementation and don't rely on 
boost.threads . It would improve consistency among boost-libraries?

  The basic demuxer-work class is copy-constructable so that it may be
used as a data member in a handler class. It is not assignable. I have
never seen this idiom in a correctly designed class, so it would be
nice to elaborate a bit on it.

Documentation:
I would like to see a clear distinction between what is considered a
public and private class. A split between basic and advanced topics
would ease understanding as well.

it would improve readability if you would omit the boost::asio
namespace throughout the tutorial. Everybody knows the library-name
when reading the tutorial. adding a line "use namespace::boost::asio"
in the beginning is sufficient.

  I did not have time to look in detail into the scatter-gather
mechanism, nor the demuxer classes, nor the ssl implementation ... so
I am only scratching the surface here. I am impressed by the library
and I would use it.

Dirk