Re: [boost] asio review

From: Christopher Kohlhoff (chris_at_[hidden])
Date: 2005-12-21 06:57:36

Hi Dirk,

--- Dirk Moermans <dirkmoermans_at_[hidden]> wrote: <snip>
> 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).

Early on asio had a separate socket_connector class, but it was
removed, mainly because:

- It does not correspond to a real operating system resource in
  BSD-based APIs.

- You often need to manipulate a socket before establishing a
  connection, e.g. binding, setting options.

- It prevented an implementation of asynchronous connect on
  Win32 using ConnectEx (still on my to-do list). By this I mean
  that if you had a separate connector it could be constructed
  to take a different demuxer to the socket being connected.
  However with ConnectEx, the completion event is delivered
  through the IO completion port (i.e. demuxer) of the socket.

> 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.

Not sure what you mean here exactly. Can you elaborate?

> 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.

Doesn't such an approach force a particular internal
implementation on the timer? The current interface permits the
timer to store the expiry in an efficient native format, if
required, and to just convert when the expires_*() functions are
accessed.

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

Fair enough.

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

The std::string constructor from const char* should take care of
these use cases, shouldn't it? What additional issue do you see
a const char* parameter addressing?

> 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

In my opinion, returning a const reference unnecessarily couples
the class's interface to its implementation. By that I mean
returning a const reference would require the host class contain
a std::string data member. That may be true of the current
implementation, but it may not always be the best option.

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

Never really thought about it, it has just been the idiomatic
usage in all code I've worked on.

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

When developing asio I did not want to require dependency on
anything other than the boost header files, whereas
boost.threads requires you to link against the library. The asio
thread implementation is completely internal though, with no
effect on the public interface, so it may use boost.threads in
the future without impacting application source code. You're
also free, of course, to use boost.threads alongside asio (e.g.
to call demuxer::run() in a separate thread).

>   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.

It behaves more like a reference, i.e. no default construction
or assigning. This is based on the requirements placed by asio
on completion handlers, i.e. CopyConstructible, but not required
to be DefaultConstructible or Assignable.

> 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.

Yep, it's on my to-do list to split the documentation in this
way.

> 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.

No worries. I'm just in the habit of fully qualifying all names.

Cheers,
Chris