2
\$\begingroup\$

I'm writing a C++ wrapper library for sockets which will hopefully be cross-platform.

It's basically two headers:

  • mizaru.hpp, which contains the wrapper classes themselves.
  • trans_layer.hpp, which provides platform agnostic function prototypes. So far they only support Linux and TCP sockets.

I think that I'm using a lot of exceptions and am not sure of a better approach for handling errors. I'm also not sure about the general architecture of the library, especially relationship between SyncSocket classes.

My main issue is the error-handling approach, but any advice on how to write cleaner code with better comments is greatly appreciated.

Questions:

  1. Should I change the way errors are handled? Are there too many exceptions being thrown?

  2. Is this code considered clean?

  3. Should I rethink the general architecture of the library, or does it make sense?

Note: it is still in early development.

The header files:

mizaru.hpp

  #ifndef __MIZARU__HPP__
  #define __MIZARU__HPP__
  #include <string>
  #include <memory>
  #include <vector>
  #include <mutex>
  #include <chrono>
  #include "mizaru_exceptions.hpp"
  #include "types.hpp"

  namespace mizaru
  {

  class IPv4
  {
  public:
    /**
     *@warning param must be in host order.
     */
    explicit IPv4 ( unsigned int host_order );
    explicit IPv4 ( std::string &string_rep );
    explicit IPv4 ( std::string &&string_rep );
    uint32_t get() const
    {
      return data;
    }
  private:
    uint32_t data;

  };


  /**
   *@brief Base class for all sockets.
   *@warning must be unlocked before destroyed.
   *@sa SyncSocketTCP
   */
  class SyncSocket
  {
  public:

    bool try_lock() noexcept;
    bool try_read_lock() noexcept;
    bool try_write_lock() noexcept;

    void lock() noexcept;
    void read_lock() noexcept;
    void write_lock() noexcept;

    void unlock() noexcept;
    void read_unlock() noexcept;
    void write_unlock() noexcept;

    /**
     *@brief this is a blocking read function.
     *@param[in] remove if false a subsequent read would read the same data.
     *@return number of bytes read.
     */
    unsigned int read ( byte_buffer &buffer, const unsigned int length, bool remove = true) throw ( SocketClosedException,
        SystemException, RemoteHungUpException );

    /**
     *@brief non blocking read function.
     *@param[in] wait_time maximum ammount of time to wait in milliseconds specify zero (or zero_time static member) for truely
     * non-blocking.
     *@param[in] remove if false a subsequent read would read the same data.
     */
    unsigned int read ( byte_buffer &buffer, const unsigned int length, const std::chrono::milliseconds &wait_time,
                        bool remove = true) throw ( SocketClosedException,
                            SystemException, RemoteHungUpException );

    /**
     *@brief blocking write function.
     *@return number of bytes written
     */
    unsigned int write ( const byte_buffer &buffer) throw ( SocketClosedException, SystemException, BufferTooLargeException,
        RemoteHungUpException );


    /**
     *@brief non-blocking write function.
     *@return number of bytes written
     *@param[in] wait_time maximum ammount of time to wait in milliseconds specify zero (or zero_time static member) for truely
     * non-blocking.
     *
     */
    unsigned int write ( const byte_buffer &buffer, const std::chrono::milliseconds &wait_time ) throw ( SocketClosedException,
        SystemException, BufferTooLargeException,
        RemoteHungUpException );


    bool poll_read ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );
    bool poll_write ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );
    bool poll_read_write ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );

    virtual ~SyncSocket();
    /**
     *@brief represents a wait_time of zero
     */
    static const std::chrono::milliseconds zero_time;
  protected:
    SyncSocket ( const IPv4 &ip, const port_t port_number, bool udp ) throw ( SystemException );
    SyncSocket ( const SyncSocket &to_copy ) noexcept;
    SyncSocket ( SyncSocket && to_move ) noexcept;
    native_socket_t handle;
    port_t port_number;
    IPv4 ip;
    std::shared_ptr<bool>closed;

  private:

    std::shared_ptr<std::recursive_mutex> read_mutex, write_mutex;

  };



  class SyncSocketTCP : public SyncSocket
  {
  public:
    SyncSocketTCP ( const IPv4 &ip, const port_t port_number, bool keep_alive = false ) throw ( SystemException, AddressException,ConnectionRefusedException,
        ConnectionTimedOutException, NetworkUnreachableException,
        FirewallException );
    SyncSocketTCP ( const SyncSocketTCP &to_copy )  noexcept;
    SyncSocketTCP ( SyncSocketTCP &&to_move ) noexcept;

  };


  class ServSockTCP
  {

  };

  class SyncSockUDP : public SyncSocket
  {
  public:

  };

  }

  #endif /* __MIZARU__HPP__ */

trans_layer.hpp

    #ifndef __TRANS_LAYER_HPP__
    #define __TRANS_LAYER_HPP__
    #include "mizaru.hpp"
    /**
     *@file trans_layer.hpp
     *@brief provides low level interface for creating and manipulating sockets.
     */

    namespace mizaru
    {

    /**
     *@namespace trans trans_layer.hpp
     *@brief contains all low level socket maniuplation functions.
     */
    namespace trans
    {


    enum Protocol {TCP, UDP};
    enum Type {IPv4, IPv6};
    enum PollType {POLL_READ, POLL_WRITE, POLL_RW};

    /**
     *@brief Represents accepted sockets.
     */
    struct accepted_socket
    {
      mizaru::IPv4 ip;
      native_socket_t socket;
      port_t port;
    };

    native_socket_t create_socket ( Protocol p, Type t ) throw ( SystemException );

    void close_socket ( native_socket_t socket ) noexcept;

    bool poll ( native_socket_t socket, PollType type, const std::chrono::milliseconds &wait_time ) throw ( SystemException );


    /**
     *@return number of bytes read.
     *@param[in] remove if not set later calles to read on same socket will return same data
     */
    unsigned int read ( native_socket_t socket, byte_buffer &buffer, const unsigned int length, bool remove )
    throw ( SystemException, RemoteHungUpException );

    /**
     *@return number of bytes written.
     */
    unsigned int write ( native_socket_t socket, const byte_buffer &buffer ) throw ( SystemException, RemoteHungUpException,
        BufferTooLargeException );


    /**
     *@brief binds a socket to an address.
     *not used directly by TCP sockets. UDP sockets will be ready for read after calling this function.
     */
    void bind ( native_socket_t socket, const mizaru::IPv4 &ip, const port_t port ) throw ( AddressException, AddressInUseException, SystemException );


    /**
     *@brief connects a TCP or UDP Socket to a remote address and port.
     * TCP will be ready for read/write , UDP for write only
     */
    void connect ( native_socket_t socket, const mizaru::IPv4 &ip, const port_t port ) throw ( AddressException, ConnectionRefusedException,
        ConnectionTimedOutException, NetworkUnreachableException,
        SystemException, FirewallException );

    /**
     *@brief must be called before accept.
     *@sa accept
     *@param[in] port network byte order
     *@param[in] queue_length listen queue_length
     */
    void prep_tcp_for_listen ( native_socket_t socket, const port_t port, int queue_length ) throw ( AddressException, AddressInUseException, SystemException );

    /**
     *@brief accepts sockets (TCP ONLY).
     *@detail acc will contain the representation of a connected (ready for IO) TCP socket handle representing the client
     *and the peer's addr and port.
     *@warning must be preped for listen
     *@sa prep_tcp_for_listen
     */
    void accept ( native_socket_t socket, accepted_socket &acc ) throw ( SystemException, FirewallException );


    void set_keep_alive ( mizaru::native_socket_t socket ) throw ( SystemException );
    }

    }


    #endif /* __TRANS_LAYER_HPP__ */

Related header:

types.hpp

  #ifndef __TYPES_HPP__
  #define __TYPES_HPP__

  namespace mizaru
  {
  typedef std::vector<unsigned char> byte_buffer;
  typedef uint16_t port_t;

  #ifdef __linux__
  typedef int native_socket_t;
  #endif
  }
  #endif /* __TYPES_HPP__ */

mizaru_exceptions.hpp

  #ifndef __MIZARU_EXCEPTIONS_HPP__
  #define __MIZARU_EXCEPTIONS_HPP__
  #include <stdexcept>
  #include <string>

  /**
   *@file mizaru_exceptions.hpp
   *@brief defines various exceptions thrown by Mizaru functions.
   */
  namespace mizaru
  {

  class MizaruException : public std::runtime_error
  {
  public:
    MizaruException ( std::string &msg ) : std::runtime_error ( msg ) {}


  };

  /**
   *@brief This exception indicates a system exception, the specific error
   * is platform specific and could indicate something like maximem ammount of files opened
   * , process file table overflow , insufficient kernal memory etc.
   */
  class SystemException : public MizaruException
  {
  public:
    SystemException ( std::string &msg ) : MizaruException ( msg ) {}
  };


  class BufferTooLargeException : public SystemException
  {
  public:
    BufferTooLargeException ( std::string &msg ) : SystemException ( msg ) {}
  };

  class AddressException : public MizaruException
  {
  public:
    AddressException ( std::string &msg ) : MizaruException ( msg ) {}
  };

  /**
   *@brief thrown if local port already in use
   */
  class AddressInUseException : public AddressException
  {
  public:
    AddressInUseException ( std::string &msg ) : AddressException ( msg ) {}
  };


  class ConnectionException : public MizaruException
  {
  public:
    ConnectionException ( std::string &msg ) : MizaruException ( msg ) {}
  };

  class ConnectionRefusedException : public ConnectionException
  {
  public:
    ConnectionRefusedException ( std::string &msg ) : ConnectionException ( msg ) {}
  };

  class ConnectionTimedOutException : public ConnectionException
  {
  public:
    ConnectionTimedOutException ( std::string &msg ) : ConnectionException ( msg ) {}
  };

  class NetworkUnreachableException : public ConnectionException
  {
  public:
    NetworkUnreachableException ( std::string &msg ) : ConnectionException ( msg ) {}
  };


  class RemoteHungUpException : public ConnectionException
  {
  public:
    RemoteHungUpException ( std::string &msg ) : ConnectionException ( msg ) {}
  };

  class SocketClosedException : public MizaruException
  {
  public:
    SocketClosedException ( std::string &msg ) : MizaruException ( msg ) {}
  };

  class FirewallException : public ConnectionException
  {
  public:
    FirewallException ( std::string &msg ) : ConnectionException ( msg ) {}
  };

  }

  #endif /* __MIZARU_EXCEPTIONS_HPP__ */
\$\endgroup\$
  • 2
    \$\begingroup\$ I removed the licence notifications, as they are just clutter for this site. Note that by posting content you are releasing it under cc-by-sa. If you don't agree to this, you should delete your post. \$\endgroup\$ – Lstor Jul 22 '13 at 14:29
  • \$\begingroup\$ Not another one. Use the boost one. \$\endgroup\$ – Martin York Jul 23 '13 at 4:57
3
\$\begingroup\$

In Addition to everything @Lstor said:

I don't like this locking design:

bool try_lock() noexcept;
bool try_read_lock() noexcept;
bool try_write_lock() noexcept;

void lock() noexcept;
void read_lock() noexcept;
void write_lock() noexcept;

void unlock() noexcept;
void read_unlock() noexcept;
void write_unlock() noexcept;

It looks like a C interface. Its hard to see how you could make that exception safe. What I would expect is to see an interface where you get a locked object and then read and/or write against the locked object.

In this interface. Why are you passing a length?

unsigned int read ( byte_buffer &buffer, const unsigned int length, const std::chrono::milliseconds &wait_time, bool remove = true)

Does the byte_buffer type not already have a length!
What does the remove parameter do?
Really do not specify an exception specification.

This is a bad idea:

bool poll_read ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );
bool poll_write ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );
bool poll_read_write ( const std::chrono::milliseconds &wait_time = zero_time ) throw ( SystemException, SocketClosedException );

You need to provide a polling interface like select or poll or epoll. The above interface limits the number of sockets you can have open to the number of threads. If you want to open thousands of sockets (like a web server) you need something like the above which monitors many connections simultaneously using a single thread.

What is this?

std::shared_ptr<bool>closed;

For my opinion on number of exceptions see: https://softwareengineering.stackexchange.com/a/118187/12917

\$\endgroup\$
  • \$\begingroup\$ the read function can be called on an empty vector , length is the number of bytes you want to extract do you think this should change? as for the locking interface I am not sure what you mean could you clarify ? the closed ptr is for copying sockets does that make sense? \$\endgroup\$ – A. H. Jul 23 '13 at 11:07
  • \$\begingroup\$ @A.H.: If you want to read 500 bytes. Your vector has to be 500 bytes long. You may as well set this before the call. If you have a generic buffer calling resize() to make this smaller and larger for multiple calls will improve performance as smaller buffers will not deallocate the memory and you can re-use rather than going back to the memory allocation library for more memory. \$\endgroup\$ – Martin York Jul 24 '13 at 3:41
  • \$\begingroup\$ @A.H.: Calling Lock() then doing something then calling Unlock(). Is C like not C++ like. You should be using RAII to automate this. One approach: When I call lock() on the stream I get back an object that I can then use for reading/writting. When I am finished with the object I let it fall out of scope and its destructor will release the lock on the stream. \$\endgroup\$ – Martin York Jul 24 '13 at 3:47
4
\$\begingroup\$

I don't have time to give any meaningful feedback about your design at the moment. I'll give some general feedback and try to answer your question about your error handling, however.

General feedback

Moral #1: Never write an exception specification.

Moral #2: Except possibly an empty one, but if I were you I’d avoid even that.

  • Don't use double underscores in preprocessor #defines (including include guards). Names that contain double underscores are reserved. Names that begin with an underscore followed by a capital letter are also reserved.
  • Sort your #includes alphabetically. Include your own headers before standard or library headers headers.
  • Shouldn't your exception constructors take std::string const& instead of just std::string&?
  • Consider using stream operations (op<< and op>>) instead of write and read respectively.
  • Avoid bool in interfaces / as function parameter. It causes code that is hard to read and hard to use correctly.
  • You are using both uint32_t and unsigned int - you should probably be consistent and use only one or the other. Remember to include <cstdint> when using std::uint32_t.

Exceptions

Using exceptions has a cost. It's fairly expensive in terms of speed, and should generally only be used for exceptional circumstances. Most of your exceptions seem reasonable.

I don't think you should worry about having too many exceptions. There's no particular increased cost of having many exception classes, unless you get to the point where code maintenance costs are increasing or readability is decreasing. You don't seem to be anywhere near that point. As long as your exception hierarchy gives you sufficient granularity while maintaining readability, like it seems your hierarchy does, you'll be fine.

\$\endgroup\$
  • \$\begingroup\$ Exceptions are slow compared to what? Yes they are slower but if there is an error not significantly compared to normal exiting a function. Now if you use it as normal flow control then yes it is very slow compared to alternatives so don't use as flow control. Using exceptions for errors is fine. \$\endgroup\$ – Martin York Jul 23 '13 at 4:51
  • \$\begingroup\$ The term expensive should generally only be used for exceptional circumstances is a silly self referential statement. Use exceptions when you need to pass control to a non local error handling code. If error code is local and control does not cross an interface boundary then error codes are fine. But anything that extends past an interface should use exception (apart from exceptional circumstances :-) ) \$\endgroup\$ – Martin York Jul 23 '13 at 4:54
  • \$\begingroup\$ See: programmers.stackexchange.com/a/118187/12917 \$\endgroup\$ – Martin York Jul 23 '13 at 4:56
  • \$\begingroup\$ thanks for the clarification about exception specification I misunderstood what they actually do \$\endgroup\$ – A. H. Jul 23 '13 at 11:12
  • \$\begingroup\$ @LokiAstari I think we generally agree on when to use exceptions and when not to (based on your post in Programmers), and I admit that my wording was a bit vague. Thanks for the input :-) \$\endgroup\$ – Lstor Jul 23 '13 at 12:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.