[trac554] Address comments in review

src/lib/asiolink/Makefile.am

@@ -17,9 +17,9 @@ libasiolink_la_SOURCES += dns_answer.h
 libasiolink_la_SOURCES += dns_lookup.h
 libasiolink_la_SOURCES += dns_server.h
 libasiolink_la_SOURCES += dns_service.h dns_service.cc
+libasiolink_la_SOURCES += dummy_io_cb.h
 libasiolink_la_SOURCES += interval_timer.h interval_timer.cc
 libasiolink_la_SOURCES += io_address.h io_address.cc
-libasiolink_la_SOURCES += io_completion_cb.h
 libasiolink_la_SOURCES += io_endpoint.h io_endpoint.cc
 libasiolink_la_SOURCES += io_error.h
 libasiolink_la_SOURCES += io_fetch.h io_fetch.cc

src/lib/asiolink/io_completion_cb.h → src/lib/asiolink/dummy_io_cb.h

@@ -12,8 +12,8 @@
 // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
 // PERFORMANCE OF THIS SOFTWARE.
 
-#ifndef __IO_COMPLETION_CB_H
-#define __IO_COMPLETION_CB_H
+#ifndef __DUMMY_IO_CB_H
+#define __DUMMY_IO_CB_H
 
 #include <iostream>
 
@@ -33,7 +33,7 @@ namespace asiolink {
 /// template parameter.  This is the reason for this class - it is the dummy
 /// template parameter.
 
-class IOCompletionCallback {
+class DummyIOCallback {
 public:
 
     /// \brief Asynchronous I/O callback method
@@ -41,9 +41,11 @@ public:
     /// \param error Unused
     /// \param length Unused
     void operator()(asio::error_code, size_t)
-    {}
+    {
+        // TODO: log an error if this method ever gets called.
+    }
 };
 
 } // namespace asiolink
 
-#endif // __IO_COMPLETION_CB_H
+#endif // __DUMMY_IO_CB_H

src/lib/asiolink/io_asio_socket.h

@@ -28,9 +28,7 @@
 
 #include <asiolink/io_error.h>
 #include <asiolink/io_socket.h>
-#include <asiolink/io_completion_cb.h>
 
-using namespace asio;
 
 namespace asiolink {
 
@@ -126,22 +124,29 @@ public:
 
     /// \brief Open AsioSocket
     ///
-    /// A call that is a no-op on UDP sockets, this opens a connection to the
-    /// system identified by the given endpoint.
+    /// Opens the socket for asynchronous I/O.  On a UDP socket, this is merely
+    /// an "open()" on the underlying socket (so completes immediately), but on
+    /// a TCP socket it also connects to the remote end (which is done as an
+    /// asynchronous operation).
+    ///
+    /// For TCP, signalling of the completion of the operation is done by
+    /// by calling the callback function in the normal way.  This could be done
+    /// for UDP (by posting en event on the event queue); however, that will
+    /// incur additional overhead in the most common case.  Instead, the return
+    /// value indicates whether the operation was asynchronous or not. If yes,
+    /// (i.e. TCP) the callback has been posted to the event queue: if no (UDP),
+    /// no callback has been posted (in which case it is up to the caller as to
+    /// whether they want to manually post the callback themself.)
     ///
     /// \param endpoint Pointer to the endpoint object.  This is ignored for
     /// a UDP socket (the target is specified in the send call), but should
     /// be of type TCPEndpoint for a TCP connection.
-    /// \param callback I/O Completion callback, called when the connect has
-    /// completed.  In the stackless coroutines model, this will be the
-    /// method containing the call to this function, allowing the operation to
-    /// resume after the socket open has completed.
+    /// \param callback I/O Completion callback, called when the operation has
+    /// completed, but only if the operation was asynchronous.
     ///
     /// \return true if an asynchronous operation was started and the caller
-    /// should yield and wait for completion, false if not. (i.e. The UDP
-    /// derived class will return false, the TCP class will return true).  This
-    /// optimisation avoids the overhead required to post a callback to the
-    /// I/O Service queue where nothing is done.
+    /// should yield and wait for completion, false if the operation was
+    /// completed synchronously and no callback was queued.
     virtual bool open(const IOEndpoint* endpoint, C& callback) = 0;
 
     /// \brief Send Asynchronously

src/lib/asiolink/io_fetch.cc

@@ -143,7 +143,22 @@ IOFetch::operator()(error_code ec, size_t length) {
 
 void
 IOFetch::stop(Result result) {
+
     if (!data_->stopped) {
+
+        // Mark the fetch as stopped to prevent other completion callbacks
+        // (invoked because of the calls to cancel()) from executing the
+        // cancel calls again.
+        //
+        // In a single threaded environment, the callbacks won't be invoked
+        // until this one completes. In a multi-threaded environment, they may
+        // well be, in which case the testing (and setting) of the stopped_
+        // variable should be done inside a mutex (and the stopped_ variable
+        // declared as "volatile").
+        //
+        // TODO: Update testing of stopped_ if threads are used.
+        data_->stopped = true;
+
         switch (result) {
             case TIME_OUT:
                 dlog("Query timed out");
@@ -170,7 +185,7 @@ IOFetch::stop(Result result) {
         }
 
         // Mark that stop() has now been called.
-        data_->stopped = true;
+
     }
 }
 

src/lib/asiolink/io_fetch.h

@@ -74,12 +74,15 @@ public:
 
     /// \brief I/O Fetch Callback
     ///
-    /// TODO: change documentation
-    /// Callback object for when the fetch itself has completed.  Note that this
-    /// is different to the IOCompletionCallback; that is used to signal the
-    /// completion of an asynchronous I/O call.  The IOFetch::Callback is called
-    /// when an upstream fetch - which may have involved several asynchronous
-    /// I/O operations - has completed.
+    /// Class of callback object for when the fetch itself has completed - an
+    /// object of this class is passed to the IOFetch constructor and its
+    /// operator() method called when the fetch completes.
+    ///
+    /// Note the difference between the two operator() methods:
+    /// - IOFetch::operator() callback is called when an asynchronous I/O has
+    ///   completed.
+    /// - IOFetch::Callback::operator() is called when an upstream fetch - which
+    ///   may have involved several asynchronous I/O operations - has completed.
     ///
     /// This is an abstract class.
     class Callback {
@@ -141,6 +144,8 @@ public:
         ///     when we terminate.  The caller is responsible for managing this
         ///     object and deleting it if necessary.
         /// \param wait Timeout for the fetch (in ms).
+        ///
+        /// TODO: May need to alter constructor (see comment 4 in Trac ticket #554)
         IOFetchData(int protocol, IOService& service,
             const isc::dns::Question& query, const IOAddress& address,
             uint16_t port, isc::dns::OutputBufferPtr& buff, Callback* cb,
@@ -192,9 +197,7 @@ public:
         const isc::dns::Question& question, const IOAddress& address,
         uint16_t port, isc::dns::OutputBufferPtr& buff, Callback* cb,
         int wait = -1);
-
-    // The default constructor and copy constructor are correct for this method.
-
+    
     /// \brief Coroutine entry point
     ///
     /// The operator() method is the method in which the coroutine code enters

src/lib/asiolink/tcp_server.cc

@@ -20,12 +20,11 @@
 
 #include <log/dummylog.h>
 
-#include <asiolink/io_completion_cb.h>
+#include <asiolink/dummy_io_cb.h>
 #include <asiolink/tcp_endpoint.h>
 #include <asiolink/tcp_socket.h>
 #include <asiolink/tcp_server.h>
 
-
 using namespace asio;
 using asio::ip::udp;
 using asio::ip::tcp;
@@ -120,10 +119,9 @@ TCPServer::operator()(error_code ec, size_t length) {
         // and takes as a template parameter a completion callback class.  As
         // TCPServer does not use these extended functions (only those defined
         // in the IOSocket base class) - but needs a TCPSocket to get hold of
-        // the underlying Boost TCP socket - use "IOCompletionCallback" -
-        // a basic callback class: it is not used but provides the appropriate
-        // signature.
-        iosock_.reset(new TCPSocket<IOCompletionCallback>(*socket_));
+        // the underlying Boost TCP socket - DummyIOCallback is used.  This
+        // provides the appropriate operator() but is otherwise functionless.
+        iosock_.reset(new TCPSocket<DummyIOCallback>(*socket_));
         io_message_.reset(new IOMessage(data_.get(), length, *iosock_, *peer_));
         bytes_ = length;
 

src/lib/asiolink/tcp_socket.h

@@ -36,10 +36,8 @@
 
 namespace asiolink {
 
-/// \brief The \c TCPSocket class is a concrete derived class of
-/// \c IOSocket that represents a TCP socket.
-///
-/// Other notes about \c TCPSocket applies to this class, too.
+/// \brief The \c TCPSocket class is a concrete derived class of \c IOAsioSocket
+/// that represents a TCP socket.
 ///
 /// \param C Callback type
 template <typename C>
@@ -50,9 +48,6 @@ private:
     TCPSocket& operator=(const TCPSocket&);
 
 public:
-    enum {
-        MAX_SIZE = 4096         // Send and receive size
-    };
     
     /// \brief Constructor from an ASIO TCP socket.
     ///

src/lib/asiolink/tests/io_fetch_unittest.cc

@@ -30,7 +30,6 @@
 #include <dns/rcode.h>
 
 #include <asiolink/io_fetch.h>
-#include <asiolink/io_completion_cb.h>
 #include <asiolink/io_service.h>
 
 using namespace asio;

src/lib/asiolink/udp_server.cc

@@ -20,14 +20,13 @@
 
 #include <log/dummylog.h>
 
-#include <asiolink/io_completion_cb.h>
+#include <asiolink/dummy_io_cb.h>
 #include <asiolink/udp_endpoint.h>
 #include <asiolink/udp_server.h>
 #include <asiolink/udp_socket.h>
 
 using namespace asio;
 using asio::ip::udp;
-using asio::ip::tcp;
 using isc::log::dlog;
 
 using namespace std;
@@ -204,15 +203,14 @@ UDPServer::operator()(error_code ec, size_t length) {
         // all these calls to "new".)
         data_->peer_.reset(new UDPEndpoint(*data_->sender_));
 
-        // The TCP socket class has been extended with asynchronous functions
+        // The UDP socket class has been extended with asynchronous functions
         // and takes as a template parameter a completion callback class.  As
-        // TCPServer does not use these extended functions (only those defined
-        // in the IOSocket base class) - but needs a TCPSocket to get hold of
-        // the underlying Boost TCP socket - use "IOCompletionCallback" -
-        // a basic callback class: it is not used but provides the appropriate
-        // signature.
+        // UDPServer does not use these extended functions (only those defined
+        // in the IOSocket base class) - but needs a UDPSocket to get hold of
+        // the underlying Boost UDP socket - DummyIOCallback is used.  This
+        // provides the appropriate operator() but is otherwise functionless.
         data_->iosock_.reset(
-            new UDPSocket<IOCompletionCallback>(*data_->socket_));
+            new UDPSocket<DummyIOCallback>(*data_->socket_));
 
         data_->io_message_.reset(new IOMessage(data_->data_.get(),
             data_->bytes_, *data_->iosock_, *data_->peer_));

src/lib/asiolink/udp_socket.h

@@ -35,10 +35,8 @@
 
 namespace asiolink {
 
-/// \brief The \c UDPSocket class is a concrete derived class of
-/// \c IOSocket that represents a UDP socket.
-///
-/// Other notes about \c TCPSocket applies to this class, too.
+/// \brief The \c UDPSocket class is a concrete derived class of \c IOAsioSocket
+/// that represents a UDP socket.
 ///
 /// \param C Callback type
 template <typename C>