Re: [boost] Re: [network] An RFC - updated

From: Peter Dimov (pdimov_at_[hidden])
Date: 2005-04-22 04:42:47

I like the proposal. It shows a certain polish that only comes with 
real-world experience with a design. ;-)

I have the following comments for now, based on a not-very-deep look at the 
document and the header:

1. network root class

In my opinion, this class is not needed and should be removed. It 
complicates the design, forces a global variable on the user, and is not 
library-friendly. For example, if lib1 and lib2 use the network, the user 
can't pass an address created by lib1 to lib2. The global state modeled by 
the network class should be an implementation detail.

2. address

The address should be a standalone class and not tied to a particular 
network. It should be a simple entity and not a hierarchical container. 
Logical to physical resolution should take a source logical address and 
produce a container of physical addresses.

The address should not be created with an URL, that is, a TCP connection to 
www.example.com should be represented by the address 
tcp:/www.example.com:80, leaving http://www.example.com reserved for the 
stream obtained by the HTTP GET / query, not for the raw protocol stream. A 
connection to the serial port should probably be represented by 
"com1:/38400,n,8,1", and so on.

I think that the UDP broadcast address should be represented by 
udp:/0.0.0.0, and the TCP loopback should be tcp:/127.0.0.1, as usual. But I 
may be wrong.

3. Minor stylistic issues

p->get_X() should be p->X(), p->new_Y() should probably be p->create_Y(), 
although it might be better to move to net::create_Y( p ) or even to Y( p ) 
and reference semantics under the hood.

The destructors should be protected.

4. MT-centric model

The general idea of an asynchronous callback dispatcher is sound, but the 
threading decision should be left to the user. The library should provide

    void net::poll( timeout tm );

that delivers any pending callbacks from the context of the thread that 
called poll, and

    void net::async_poll( error_callback );

that acts "as if" it executes net::poll( infinite ) repeatedly in one or 
more background threads. (error_callback will be called on errors; the 
synchronous variant will probably throw an exception instead.)

Whether the library uses multiple threads under the hood, or how many, is 
implementation defined, even if async_poll is not called; this depends on 
which model delivers the best performance on the specific platform.

5. read_later, etc

I think that read_later should be dropped. async_read is enough, in my 
opinion.

The functionality of write_later should be achievable with a call to 
async_write with size 0; write_later can be dropped, too.

async_read should not take a buffer; instead, the callback should receive a 
pointer to a buffer managed by the library that is guaranteed to be valid 
for the duration of the callback. (Not by default, at least.)

async_write (by default) should not assume that the passed buffer stays 
valid after async_write returns.

Rationale: buffer management is a pain with multiple callbacks active ;-)

That's it for now. Please don't take these as criticisms and wherever you 
see "should" read "in my humble opinion, the design probably could be 
enhanced by".

Thanks for reading.