widelands-dev team mailing list archive

[Notabilis <notabilis27@xxxxxx>]

Thanks for the review, I will address most of it.
I commented some of your comments, on two points I disagree with you and would prefer the way it currently is. Feel free to insist if you think I am wrong.

Diff comments:

> 
> === modified file 'src/network/netclient.cc'
> --- src/network/netclient.cc	2017-05-11 10:45:44 +0000
> +++ src/network/netclient.cc	2017-05-26 20:56:15 +0000
> @@ -17,20 +18,25 @@
>  NetClient::~NetClient() {
>  	if (is_connected())
>  		close();
> -	if (sockset_ != nullptr)
> -		SDLNet_FreeSocketSet(sockset_);
>  }
>  
>  bool NetClient::is_connected() const {
> -	return sock_ != nullptr;
> +	return socket_.is_open();
>  }
>  
>  void NetClient::close() {
>  	if (!is_connected())
>  		return;
> -	SDLNet_TCP_DelSocket(sockset_, sock_);
> -	SDLNet_TCP_Close(sock_);
> -	sock_ = nullptr;
> +	boost::system::error_code ec;

That's not actually resolving it, but getting some internal data. The error shouldn't occur at this place as far as I know.
We aren't storing the NetAddress, so it is not available here.

> +	boost::asio::ip::tcp::endpoint remote = socket_.remote_endpoint(ec);
> +	if (!ec) {
> +		log("[NetClient] Closing network socket connected to %s:%i.\n",
> +			remote.address().to_string().c_str(), remote.port());
> +	} else {
> +		log("[NetClient] Closing network socket.\n");
> +	}
> +	socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
> +	socket_.close(ec);
>  }
>  
>  bool NetClient::try_receive(RecvPacket* packet) {
> 
> === modified file 'src/network/nethost.cc'
> --- src/network/nethost.cc	2017-05-11 10:45:44 +0000
> +++ src/network/nethost.cc	2017-05-26 20:56:15 +0000
> @@ -47,68 +53,150 @@
>  		// Not connected anyway
>  		return;
>  	}
> -	SDLNet_TCP_DelSocket(sockset_, iter_client->second.socket);
> -	SDLNet_TCP_Close(iter_client->second.socket);
> +	boost::system::error_code ec;
> +	boost::asio::ip::tcp::endpoint remote = iter_client->second.socket.remote_endpoint(ec);
> +	if (!ec) {
> +		log("[NetHost] Closing network connection to %s:%i.\n",
> +			remote.address().to_string().c_str(), remote.port());
> +	} else {
> +		log("[NetHost] Closing network connection to some client.\n");
> +	}
> +	iter_client->second.socket.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ec);
> +	iter_client->second.socket.close(ec);
>  	clients_.erase(iter_client);
>  }
>  
>  bool NetHost::try_accept(ConnectionId* new_id) {
>  	if (!is_listening())
>  		return false;
> +	boost::system::error_code ec;
> +	boost::asio::ip::tcp::socket socket(io_service_);

Yes, but we would have to take care of freeing it ourselves. Is there a problem with move() ?

> +	if (acceptor_v4_.is_open()) {
> +		acceptor_v4_.accept(socket, ec);
> +		if (ec == boost::asio::error::would_block) {
> +			// No client wants to connect
> +			// New socket don't need to be closed since it isn't open yet
> +		} else if (ec) {
> +			// Some other error, close the acceptor
> +			log("[NetHost] No longer listening for IPv4 connections due to error: %s.\n",
> +				ec.message().c_str());
> +			acceptor_v4_.close(ec);
> +		} else {
> +			log("[NetHost]: Accepting IPv4 connection from %s.\n",
> +				socket.remote_endpoint().address().to_string().c_str());
> +		}
> +	}
> +	if (acceptor_v6_.is_open() && !socket.is_open()) {
> +		// IPv4 did not get a connection
> +		acceptor_v6_.accept(socket, ec);
> +		if (ec == boost::asio::error::would_block) {
> +			// No client wants to connect
> +		} else if (ec) {
> +			log("[NetHost] No longer listening for IPv6 connections due to error: %s.\n",
> +				ec.message().c_str());
> +			acceptor_v6_.close(ec);
> +		} else {
> +			log("[NetHost]: Accepting IPv6 connection from %s.\n",
> +				socket.remote_endpoint().address().to_string().c_str());
> +		}
> +	}
>  
> -	TCPsocket sock = SDLNet_TCP_Accept(svrsock_);
> -	// No client wants to connect
> -	if (sock == nullptr)
> +	if (!socket.is_open()) {
> +		// No new connection
>  		return false;
> -	SDLNet_TCP_AddSocket(sockset_, sock);
> +	}
> +
> +	socket.non_blocking(true);
> +
>  	ConnectionId id = next_id_++;
>  	assert(id > 0);
>  	assert(clients_.count(id) == 0);
> -	clients_.insert(std::make_pair(id, Client(sock)));
> +	clients_.insert(std::make_pair(id, Client(std::move(socket))));
>  	assert(clients_.count(id) == 1);
>  	*new_id = id;
>  	return true;
>  }
>  
>  bool NetHost::try_receive(const ConnectionId id, RecvPacket* packet) {
> -
>  	// Always read all available data into buffers
> -	uint8_t buffer[512];
> -	while (SDLNet_CheckSockets(sockset_, 0) > 0) {
> -		for (auto& e : clients_) {
> -			if (SDLNet_SocketReady(e.second.socket)) {
> -				const int32_t bytes = SDLNet_TCP_Recv(e.second.socket, buffer, sizeof(buffer));
> -				if (bytes <= 0) {
> -					// Error while receiving
> -					close(e.first);
> -					// We have to run the for-loop again since we modified the map
> -					break;
> -				}
> +	uint8_t buffer[kNetworkBufferSize];
>  
> -				e.second.deserializer.read_data(buffer, bytes);
> -			}
> +	boost::system::error_code ec;
> +	for (auto it = clients_.begin(); it != clients_.end(); ) {
> +		size_t length = it->second.socket.read_some(boost::asio::buffer(buffer, kNetworkBufferSize), ec);
> +		if (ec == boost::asio::error::would_block) {
> +			// Nothing to read
> +			assert(length == 0);
> +			++it;
> +			continue;
> +		} else if (ec) {
> +			assert(length == 0);
> +			// Connection closed or some error, close the socket
> +			// close() will remove the client from the map so we have to increment the iterator first
> +			ConnectionId id_to_remove = it->first;
> +			++it;
> +			log("[NetHost] Error when receiving from a client, closing connection: %s.\n",
> +				ec.message().c_str());
> +			close(id_to_remove);
> +			continue;
>  		}
> +		assert(length > 0);
> +		assert(length <= kNetworkBufferSize);
> +		// Read something
> +		it->second.deserializer.read_data(buffer, length);
> +		++it;
>  	}
>  
>  	// Now check whether there is data for the requested client
>  	if (!is_connected(id))
>  		return false;
>  
> -	// Get one packet from the deserializer
> +	// Try to get one packet from the deserializer
>  	return clients_.at(id).deserializer.write_packet(packet);
>  }
>  
>  void NetHost::send(const ConnectionId id, const SendPacket& packet) {
> +	boost::system::error_code ec;
>  	if (is_connected(id)) {
> -		SDLNet_TCP_Send(clients_.at(id).socket, packet.get_data(), packet.get_size());
> -	}
> -}
> -
> -NetHost::NetHost(const uint16_t port) : svrsock_(nullptr), sockset_(nullptr), next_id_(1) {
> -
> -	IPaddress myaddr;
> -	SDLNet_ResolveHost(&myaddr, nullptr, port);
> -	svrsock_ = SDLNet_TCP_Open(&myaddr);
> -	// Maximal 16 sockets! This mean we can have at most 15 clients_ in our game (+ metaserver)
> -	sockset_ = SDLNet_AllocSocketSet(16);
> +		size_t written = boost::asio::write(clients_.at(id).socket,
> +											boost::asio::buffer(packet.get_data(), packet.get_size()), ec);
> +		// This one is an assertion of mine, I am not sure if it will hold
> +		// If it doesn't, set the socket to blocking before writing
> +		assert(ec != boost::asio::error::would_block);
> +		assert(written == packet.get_size() || ec);
> +		if (ec) {
> +			log("[NetHost] Error when sending to a client, closing connection: %s.\n", ec.message().c_str());
> +			close(id);
> +		}
> +	}
> +}
> +
> +NetHost::NetHost(const uint16_t port)
> +	: clients_(), next_id_(1), io_service_(), acceptor_v4_(io_service_), acceptor_v6_(io_service_) {
> +
> +	if (open_acceptor(&acceptor_v4_, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v4(), port))) {
> +		log("[NetHost]: Opening a listening IPv4 socket on TCP port %u\n", port);
> +	}
> +	if (open_acceptor(&acceptor_v6_, boost::asio::ip::tcp::endpoint(boost::asio::ip::tcp::v6(), port))) {
> +		log("[NetHost]: Opening a listening IPv6 socket on TCP port %u\n", port);
> +	}
> +}
> +
> +bool NetHost::open_acceptor(boost::asio::ip::tcp::acceptor *acceptor,
> +						const boost::asio::ip::tcp::endpoint& endpoint) {
> +	try {
> +		acceptor->open(endpoint.protocol());
> +		acceptor->non_blocking(true);
> +		const boost::asio::socket_base::reuse_address option_reuse(true);
> +		acceptor->set_option(option_reuse);
> +		if (endpoint.protocol() == boost::asio::ip::tcp::v6()) {
> +			const boost::asio::ip::v6_only option_v6only(true);
> +			acceptor->set_option(option_v6only);

On some systems (linux) it can, on others (windows) it can't. So disabling the option here allows to use the same code independent of the operating system in the rest of the class.

> +		}
> +		acceptor->bind(endpoint);
> +		acceptor->listen(boost::asio::socket_base::max_connections);
> +		return true;
> +	} catch (const boost::system::system_error&) {
> +		return false;
> +	}
>  }
> 
> === modified file 'src/network/network.h'
> --- src/network/network.h	2017-05-23 21:07:47 +0000
> +++ src/network/network.h	2017-05-26 20:56:15 +0000
> @@ -36,6 +35,42 @@
>  class Deserializer;
>  class FileRead;
>  
> +/**
> + * Simple structure to hold the IP address and port of a server.
> + * This structure must not contain a hostname but only IP addresses.
> + */
> +struct NetAddress {
> +	/**
> +	 * Tries to resolve the given hostname to an IPv4 address.
> +	 * \param[out] addr An NetAddress structure to write the result to,
> +	 *                  if resolution succeeds.
> +	 * \param hostname The name of the host.
> +	 * \param port The port on the host.
> +	 * \return \c True if the resolution succeeded, \c false otherwise.
> +	 */
> +	static bool resolve_to_v4(NetAddress *addr, const std::string& hostname, uint16_t port);
> +
> +	/**
> +	 * Tries to resolve the given hostname to an IPv6 address.
> +	 * \param[out] addr An NetAddress structure to write the result to,
> +	 *                  if resolution succeeds.
> +	 * \param hostname The name of the host.
> +	 * \param port The port on the host.
> +	 * \return \c True if the resolution succeeded, \c false otherwise.
> +	 */
> +	static bool resolve_to_v6(NetAddress *addr, const std::string& hostname, uint16_t port);
> +
> +	/**
> +	 * Returns whether the stored IP is in IPv6 format.
> +	 * @return \c true if the stored IP is in IPv6 format, \c false otherwise.
> +	 *   If it isn't an IPv6 address, it is an IPv4 address.
> +	 */
> +	bool is_ipv6() const;
> +
> +	std::string ip;

Actually, I like the idea! It wasn't possible when I created the struct but in its current version it is and even makes things easier.

> +	uint16_t port;
> +};
> +
>  struct SyncCallback {
>  	virtual ~SyncCallback() {
>  	}


-- 
https://code.launchpad.net/~widelands-dev/widelands/net-boost-asio/+merge/324364
Your team Widelands Developers is subscribed to branch lp:~widelands-dev/widelands/net-boost-asio.