documentation and minor fixes for async ops

7 files changed, 149 insertions, 80 deletions

diff --git a/src/lib/tls/asio/asio_async_base.h b/src/lib/tls/asio/asio_async_base.h
index 8e74c5647..23707af5b 100644
--- a/src/lib/tls/asio/asio_async_base.h
+++ b/src/lib/tls/asio/asio_async_base.h
@@ -21,6 +21,40 @@ namespace Botan {
 
 namespace TLS {
 
+/**
+ * Base class for asynchronous stream operations.
+ *
+ * Asynchronous operations, used for example to implement an interface for boost::asio::async_read_some and
+ * boost::asio::async_write_some, are based on boost::asio::coroutines.
+ * Derived operations should implement a call operator and invoke it with the correct parameters upon construction. The
+ * call operator needs to make sure that the user-provided handler is not called directly. Typically, yield / reenter is
+ * used for this in the following fashion:
+ *
+ * ```
+ * void operator()(boost::system::error_code ec, std::size_t bytes_transferred, bool isContinuation = true)
+ *    {
+ *    reenter(this)
+ *       {
+ *       // operation specific logic, repeatedly interacting with the stream_core and the next_layer (socket)
+ *
+ *       // make sure intermediate initiating function is called
+ *       if(!isContinuation)
+ *          {
+ *          yield next_layer.async_operation(empty_buffer, this);
+ *          }
+ *
+ *       // call the completion handler
+ *       complete_now(error_code, bytes_transferred);
+ *       }
+ *    }
+ * ```
+ *
+ * Once the operation is completed and ready to call the completion handler it checks if an intermediate initiating
+ * function has been called using the `isContinuation` parameter. If not, it will call an asynchronous operation, such
+ * as `async_read_some`, with and empty buffer, set the object itself as the handler, and `yield`. As a result, the call
+ * operator will be invoked again, this time as a continuation, and will jump to the location where it yielded before
+ * using `reenter`. It is now safe to call the handler function via `complete_now`.
+ */
 template <class Handler, class Executor1, class Allocator>
 struct AsyncBase : boost::asio::coroutine
    {
@@ -45,6 +79,13 @@ struct AsyncBase : boost::asio::coroutine
          {
          }
 
+      /**
+       * Call the completion handler.
+       *
+       * This function should only be called after an intermediate initiating function has been called.
+       *
+       * @param args Arguments forwarded to the completion handler function.
+       */
       template<class... Args>
       void complete_now(Args&& ... args)
          {
@@ -55,8 +96,10 @@ struct AsyncBase : boost::asio::coroutine
       Handler m_handler;
       boost::asio::executor_work_guard<Executor1> m_work_guard_1;
    };
-}
-}
+
+}  // namespace TLS
+
+}  // namespace Botan
 
 #endif // BOOST_VERSION
 #endif // BOTAN_HAS_TLS && BOTAN_HAS_BOOST_ASIO
diff --git a/src/lib/tls/asio/asio_async_handshake_op.h b/src/lib/tls/asio/asio_async_handshake_op.h
index d633fd127..a392fd649 100644
--- a/src/lib/tls/asio/asio_async_handshake_op.h
+++ b/src/lib/tls/asio/asio_async_handshake_op.h
@@ -28,6 +28,14 @@ namespace TLS {
 template <class Handler, class Stream, class Allocator = std::allocator<void>>
 struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::executor_type, Allocator>
    {
+      /**
+       * Construct and invoke an AsyncHandshakeOperation.
+       *
+       * @param handler Handler function to be called upon completion.
+       * @param stream The stream from which the data will be read
+       * @param core The stream's core; used to extract decrypted data.
+       * @param ec Optional error code; used to report an error to the handler function.
+       */
       template<class HandlerT>
       AsyncHandshakeOperation(
          HandlerT&& handler,
@@ -49,8 +57,8 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
          {
          reenter(this)
             {
-            // process tls packets from socket first
-            if(bytesTransferred > 0)
+            // Provide TLS data from the core to the TLS::Channel
+            if(bytesTransferred > 0 && !ec)
                {
                boost::asio::const_buffer read_buffer {m_core.input_buffer.data(), bytesTransferred};
                try
@@ -63,13 +71,13 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
                   }
                }
 
-            // send tls packets
+            // Write TLS data that TLS::Channel has provided to the core
             if(m_core.hasDataToSend() && !ec)
                {
-               // \note: we construct `AsyncWriteOperation` with 0 as its last parameter (`plainBytesTransferred`).
-               //        This operation will eventually call `*this` as its own handler, passing the 0 back to this call
-               //        operator. This is necessary because, the check of `bytesTransferred > 0` assumes that
-               //        `bytesTransferred` bytes were just read and are in the cores input_buffer for further processing.
+               // Note: we construct `AsyncWriteOperation` with 0 as its last parameter (`plainBytesTransferred`).
+               // This operation will eventually call `*this` as its own handler, passing the 0 back to this call
+               // operator. This is necessary because the check of `bytesTransferred > 0` assumes that
+               // `bytesTransferred` bytes were just read and are in the core's input_buffer for further processing.
                AsyncWriteOperation<
                AsyncHandshakeOperation<typename std::decay<Handler>::type, Stream, Allocator>,
                                        Stream,
@@ -78,7 +86,7 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
                return;
                }
 
-            // we need more tls data from the socket
+            // Read more data from the socket
             if(!m_stream.native_handle()->is_active() && !ec)
                {
                m_stream.next_layer().async_read_some(m_core.input_buffer, std::move(*this));
@@ -87,9 +95,8 @@ struct AsyncHandshakeOperation : public AsyncBase<Handler, typename Stream::exec
 
             if(!isContinuation)
                {
-               // this 0 byte read completes immediately. `yield` causes the coroutine to reenter the function after
-               // this read, enabling us to call the handler, while respecting asios guarantee that the handler will not
-               // be called without an intermediate initiating function
+               // Make sure the handler is not called without an intermediate initiating function.
+               // "Reading" into a zero-byte buffer will complete immediately.
                m_ec = ec;
                yield m_stream.next_layer().async_read_some(boost::asio::mutable_buffer(), std::move(*this));
                ec = m_ec;
diff --git a/src/lib/tls/asio/asio_async_read_op.h b/src/lib/tls/asio/asio_async_read_op.h
index 598ffdc7d..3863dd5b3 100644
--- a/src/lib/tls/asio/asio_async_read_op.h
+++ b/src/lib/tls/asio/asio_async_read_op.h
@@ -28,6 +28,15 @@ namespace TLS {
 template <class Handler, class Stream, class MutableBufferSequence, class Allocator = std::allocator<void>>
 struct AsyncReadOperation : public AsyncBase<Handler, typename Stream::executor_type, Allocator>
    {
+      /**
+       * Construct and invoke an AsyncWriteOperation.
+       *
+       * @param handler Handler function to be called upon completion.
+       * @param stream The stream from which the data will be read
+       * @param core The stream's core; used to extract decrypted data.
+       * @param buffers The buffers into which the data will be read.
+       * @param ec Optional error code; used to report an error to the handler function.
+       */
       template <class HandlerT>
       AsyncReadOperation(HandlerT&& handler,
                          Stream& stream,
@@ -42,7 +51,7 @@ struct AsyncReadOperation : public AsyncBase<Handler, typename Stream::executor_
          , m_buffers(buffers)
          , m_decodedBytes(0)
          {
-         this->operator()(ec, m_decodedBytes, false);
+         this->operator()(ec, std::size_t(0), false);
          }
 
       AsyncReadOperation(AsyncReadOperation&&) = default;
@@ -82,7 +91,7 @@ struct AsyncReadOperation : public AsyncBase<Handler, typename Stream::executor_
             if(!isContinuation)
                {
                // Make sure the handler is not called without an intermediate initiating function.
-               // "Reading" into a zero-byte buffer will "return" immediately.
+               // "Reading" into a zero-byte buffer will complete immediately.
                m_ec = ec;
                yield m_stream.next_layer().async_read_some(boost::asio::mutable_buffer(), std::move(*this));
                ec = m_ec;
@@ -97,7 +106,7 @@ struct AsyncReadOperation : public AsyncBase<Handler, typename Stream::executor_
       StreamCore&           m_core;
       MutableBufferSequence m_buffers;
 
-      size_t                    m_decodedBytes;
+      std::size_t               m_decodedBytes;
       boost::system::error_code m_ec;
    };
 
diff --git a/src/lib/tls/asio/asio_async_write_op.h b/src/lib/tls/asio/asio_async_write_op.h
index 28fa13931..121480d18 100644
--- a/src/lib/tls/asio/asio_async_write_op.h
+++ b/src/lib/tls/asio/asio_async_write_op.h
@@ -27,54 +27,69 @@ namespace TLS {
 template <typename Handler, class Stream, class Allocator = std::allocator<void>>
 struct AsyncWriteOperation : public AsyncBase<Handler, typename Stream::executor_type, Allocator>
    {
-   template <class HandlerT>
-   AsyncWriteOperation(HandlerT&& handler,
-                       Stream& stream,
-                       StreamCore& core,
-                       std::size_t plainBytesTransferred,
-                       const boost::system::error_code& ec = {})
-      : AsyncBase<Handler, typename Stream::executor_type, Allocator>(
-           std::forward<HandlerT>(handler),
-           stream.get_executor())
-      , m_stream(stream)
-      , m_core(core)
-      , m_plainBytesTransferred(plainBytesTransferred)
-      {
-      this->operator()(ec, std::size_t(0), false);
-      }
-
-   AsyncWriteOperation(AsyncWriteOperation&&) = default;
-
-   void operator()(boost::system::error_code ec, std::size_t bytes_transferred, bool isContinuation = true)
-      {
-      reenter(this)
+      /**
+       * Construct and invoke an AsyncWriteOperation.
+       *
+       * @param handler Handler function to be called upon completion.
+       * @param stream The stream from which the data will be read
+       * @param core The stream's core; used to extract decrypted data.
+       * @param plainBytesTransferred Number of bytes to be reported to the user-provided handler function as
+       *                              bytes_transferred. This needs to be provided since the amount of plaintext data
+       *                              consumed from the input buffer can differ from the amount of encrypted data written
+       *                              to the next layer.
+       * @param ec Optional error code; used to report an error to the handler function.
+       */
+      template <class HandlerT>
+      AsyncWriteOperation(HandlerT&& handler,
+                          Stream& stream,
+                          StreamCore& core,
+                          std::size_t plainBytesTransferred,
+                          const boost::system::error_code& ec = {})
+         : AsyncBase<Handler, typename Stream::executor_type, Allocator>(
+              std::forward<HandlerT>(handler),
+              stream.get_executor())
+         , m_stream(stream)
+         , m_core(core)
+         , m_plainBytesTransferred(plainBytesTransferred)
          {
-         m_core.consumeSendBuffer(bytes_transferred);
+         this->operator()(ec, std::size_t(0), false);
+         }
 
-         if(m_core.hasDataToSend() && !ec)
-            {
-            m_stream.next_layer().async_write_some(m_core.sendBuffer(), std::move(*this));
-            return;
-            }
+      AsyncWriteOperation(AsyncWriteOperation&&) = default;
 
-         if(!isContinuation)
+      void operator()(boost::system::error_code ec, std::size_t bytes_transferred, bool isContinuation = true)
+         {
+         reenter(this)
             {
-            m_ec = ec;
-            yield m_stream.next_layer().async_write_some(boost::asio::const_buffer(), std::move(*this));
-            ec = m_ec;
+            m_core.consumeSendBuffer(bytes_transferred);
+
+            if(m_core.hasDataToSend() && !ec)
+               {
+               m_stream.next_layer().async_write_some(m_core.sendBuffer(), std::move(*this));
+               return;
+               }
+
+            if(!isContinuation)
+               {
+               // Make sure the handler is not called without an intermediate initiating function.
+               // "Writing" to a zero-byte buffer will complete immediately.
+               m_ec = ec;
+               yield m_stream.next_layer().async_write_some(boost::asio::const_buffer(), std::move(*this));
+               ec = m_ec;
+               }
+
+            // The size of the sent TLS record can differ from the size of the payload due to TLS encryption. We need to
+            // tell the handler how many bytes of the original data we already processed.
+            this->complete_now(ec, m_plainBytesTransferred);
             }
-
-         // the size of the sent TLS record can differ from the size of the payload due to TLS encryption. We need to tell
-         // the handler how many bytes of the original data we already processed.
-         this->complete_now(ec, m_plainBytesTransferred);
          }
-      }
 
-   Stream&     m_stream;
-   StreamCore& m_core;
+   private:
+      Stream&     m_stream;
+      StreamCore& m_core;
 
-   std::size_t m_plainBytesTransferred;
-   boost::system::error_code m_ec;
+      std::size_t               m_plainBytesTransferred;
+      boost::system::error_code m_ec;
    };
 
 }  // namespace TLS
diff --git a/src/lib/tls/asio/asio_stream.h b/src/lib/tls/asio/asio_stream.h
index c3ed68489..3d9e46ed3 100644
--- a/src/lib/tls/asio/asio_stream.h
+++ b/src/lib/tls/asio/asio_stream.h
@@ -423,8 +423,7 @@ class Stream : public StreamBase<Channel>
       std::size_t write_some(const ConstBufferSequence& buffers,
                              boost::system::error_code& ec)
          {
-         std::size_t sent;
-         sent = tls_encrypt_some(buffers, ec);
+         std::size_t sent = tls_encrypt_some(buffers, ec);
          if(ec)
             { return 0; }
 
@@ -438,6 +437,7 @@ class Stream : public StreamBase<Channel>
       /**
        * Write some data to the stream. The function call will block until one or more bytes of data has been written
        * successfully, or until an error occurs.
+       *
        * @param buffers The data to be written.
        * @return The number of bytes written.
        * @throws boost::system::system_error if error occured
@@ -453,6 +453,7 @@ class Stream : public StreamBase<Channel>
 
       /**
        * Start an asynchronous write. The function call always returns immediately.
+       *
        * @param buffers The data to be written.
        * @param handler The handler to be called when the write operation completes. Copies will be made of the handler
        *        as required. The equivalent function signature of the handler must be:
@@ -467,12 +468,12 @@ class Stream : public StreamBase<Channel>
 
          boost::asio::async_completion<WriteHandler, void(boost::system::error_code, std::size_t)> init(handler);
 
-         std::size_t sent;
          boost::system::error_code ec;
-         sent = tls_encrypt_some(buffers, ec);
+         std::size_t sent = tls_encrypt_some(buffers, ec);
          if(ec)
             {
-            // we can't be sure how many bytes were commited here, so clear the send_buffer and try again
+            // we cannot be sure how many bytes were committed here
+            // so clear the send_buffer and let the AsyncWriteOperation call the handler with the error_code set
             this->m_core.clearSendBuffer();
             Botan::TLS::AsyncWriteOperation<typename std::decay<WriteHandler>::type, Stream>
             op{std::move(init.completion_handler), *this, this->m_core, std::size_t(0), ec};
@@ -487,11 +488,12 @@ class Stream : public StreamBase<Channel>
 
       /**
        * Start an asynchronous read. The function call always returns immediately.
+       *
        * @param buffers The buffers into which the data will be read. Although the buffers object may be copied as
-       *                 necessary, ownership of the underlying buffers is retained by the caller, which must guarantee
-       *                 that they remain valid until the handler is called.
-       * @param handler The handler to be called when the read operation completes.
-       *                The equivalent function signature of the handler must be:
+       *                necessary, ownership of the underlying buffers is retained by the caller, which must guarantee
+       *                that they remain valid until the handler is called.
+       * @param handler The handler to be called when the read operation completes. The equivalent function signature of
+       *                the handler must be:
        *                void(boost::system::error_code, std::size_t)
        */
       template <typename MutableBufferSequence, typename ReadHandler>
@@ -504,11 +506,7 @@ class Stream : public StreamBase<Channel>
          boost::asio::async_completion<ReadHandler, void(boost::system::error_code, std::size_t)> init(handler);
 
          AsyncReadOperation<typename std::decay<ReadHandler>::type, Stream, MutableBufferSequence>
-         op{std::move(init.completion_handler),
-            *this,
-            this->m_core,
-            buffers};
-
+         op{std::move(init.completion_handler), *this, this->m_core, buffers};
          return init.result.get();
          }
 
@@ -549,7 +547,7 @@ class Stream : public StreamBase<Channel>
          {
          std::size_t sent = 0;
          // NOTE: This is not asynchronous: it encrypts the data synchronously.
-         // Only writing on the socket is asynchronous.
+         // Only writing to the socket is asynchronous.
          for(auto it = boost::asio::buffer_sequence_begin(buffers);
                it != boost::asio::buffer_sequence_end(buffers);
                it++)
@@ -578,9 +576,9 @@ class Stream : public StreamBase<Channel>
       StreamLayer m_nextLayer;
    };
 
-} // TLS
+}  // namespace TLS
 
-} // namespace Botan
+}  // namespace Botan
 
 #endif // BOOST_VERSION
 #endif // BOTAN_HAS_TLS && BOTAN_HAS_BOOST_ASIO
diff --git a/src/lib/tls/asio/asio_stream_base.h b/src/lib/tls/asio/asio_stream_base.h
index adf3b8386..a4da63b26 100644
--- a/src/lib/tls/asio/asio_stream_base.h
+++ b/src/lib/tls/asio/asio_stream_base.h
@@ -29,8 +29,7 @@ enum handshake_type
    server
    };
 
-
-/** \brief Base class for all Botan::TLS::Stream implementations.
+/** Base class for all Botan::TLS::Stream implementations.
  *
  * This template must be specialized for all the Botan::TLS::Channel to be used.
  * Currently it only supports the Botan::TLS::Client channel that impersonates
diff --git a/src/lib/tls/asio/asio_stream_core.h b/src/lib/tls/asio/asio_stream_core.h
index 2c4e44717..b5cba9bce 100644
--- a/src/lib/tls/asio/asio_stream_core.h
+++ b/src/lib/tls/asio/asio_stream_core.h
@@ -35,14 +35,13 @@ struct StreamCore : public Botan::TLS::Callbacks
 
       virtual ~StreamCore() = default;
 
-      void tls_emit_data(const uint8_t data[], size_t size) override
+      void tls_emit_data(const uint8_t data[], std::size_t size) override
          {
          m_send_buffer.commit(
             boost::asio::buffer_copy(m_send_buffer.prepare(size), boost::asio::buffer(data, size)));
          }
 
-      void tls_record_received(uint64_t, const uint8_t data[],
-                               size_t size) override
+      void tls_record_received(uint64_t, const uint8_t data[], std::size_t size) override
          {
          // TODO: It would be nice to avoid this buffer copy. However, we need to deal with the case that the receive
          // buffer provided by the caller is smaller than the decrypted record.
@@ -60,8 +59,7 @@ struct StreamCore : public Botan::TLS::Callbacks
             }
          }
 
-      std::chrono::milliseconds
-      tls_verify_cert_chain_ocsp_timeout() const override
+      std::chrono::milliseconds tls_verify_cert_chain_ocsp_timeout() const override
          {
          return std::chrono::milliseconds(1000);
          }