Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add C++17 UDPServer ID handling #260

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add C++17 UDPServer ID handling #260

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f431aca
Adding C++17 UDPServer ID handling
PioLing Dec 6, 2024
680bb97
Improve the UDP server encapsulation and remove redundant constructor…
PioLing Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/Network/UdpServer.cpp
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review for src/Network/UdpServer.cpp:

Code Review: Patch to src/Network/UdpServer.cpp

Summary

This patch modifies the UdpServer class in the ZLToolKit project to improve UDP session ID handling using C++17 features. The primary changes involve using std::array instead of std::string for PeerIdType and using std::unordered_map with a custom type alias for better type safety.

Detailed Feedback

Code Overview

The patch refactors the PeerIdType to use std::array<uint8_t, PEERTYPELEN> which is more efficient and type-safe than the previous std::string. It also introduces a type alias SessionMapType for std::unordered_map<PeerIdType, SessionHelper::Ptr>, improving readability and maintainability. The changes aim to enhance performance and code clarity.

Strengths

  • Improved Type Safety: Using std::array for PeerIdType eliminates potential issues related to string manipulation and improves type safety.
  • Enhanced Readability: The SessionMapType alias makes the code easier to read and understand.
  • Potential Performance Improvement: std::array might offer slight performance gains compared to std::string in this context, especially for memory allocation and access.

Areas for Improvement

1. makeSockId Function

  • Issue: The assert(0) in the default case of the switch statement is not ideal. While it indicates an error, it abruptly terminates the program. A more robust approach would be to throw an exception or return an error code.
  • Suggestion: Replace assert(0) with a more graceful error handling mechanism. For example, throw a std::runtime_error exception with a descriptive message.
  • Example:
static UdpServer::PeerIdType makeSockId(sockaddr *addr, int) {
    UdpServer::PeerIdType ret(PEERTYPELEN, ' ');
    switch (addr->sa_family) {
        case AF_INET: {
            // ... existing code ...
            return ret;
        }
        case AF_INET6: {
            // ... existing code ...
            return ret;
        }
        default:
            throw std::runtime_error("Unsupported address family in makeSockId");
    }
}

2. Error Handling and Logging

  • Issue: While the code includes some logging, more detailed error handling and logging would improve debugging and troubleshooting. Specifically, consider logging the address family when an unsupported address family is encountered.
  • Suggestion: Add more informative log messages, especially in error scenarios. Consider using different log levels (e.g., ErrorL, WarnL) to categorize messages.

3. Const Correctness

  • Issue: The makeSockId function takes sockaddr *addr as a non-const parameter, even though it doesn't modify the address.
  • Suggestion: Change the parameter to const sockaddr *addr to improve const correctness.

4. SessionMapType Scope

  • Issue: The SessionMapType alias is defined locally within the UdpServer class. If this type is used elsewhere, it might be better to define it in a more accessible location (e.g., a header file).
  • Suggestion: Consider moving the SessionMapType alias to a header file if it's used outside of UdpServer.cpp.

Conclusion

The patch is a positive step towards improving the codebase. The use of std::array and the SessionMapType alias enhances type safety and readability. However, addressing the suggested improvements in error handling, logging, and const correctness will further enhance the robustness and maintainability of the code. The suggestion to potentially move SessionMapType depends on its usage elsewhere in the project.

TRANS_BY_GITHUB_AI_ASSISTANT

Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,9 @@
static constexpr auto kUdpDelayCloseMS = 3 * 1000;

static UdpServer::PeerIdType makeSockId(sockaddr *addr, int) {
UdpServer::PeerIdType ret;
UdpServer::PeerIdType ret(PEERTYPELEN, ' ');
switch (addr->sa_family) {
case AF_INET : {
ret.resize(18);
ret[0] = ((struct sockaddr_in *) addr)->sin_port >> 8;
ret[1] = ((struct sockaddr_in *) addr)->sin_port & 0xFF;
//ipv4地址统一转换为ipv6方式处理 [AUTO-TRANSLATED:ad7cf8c3]
Expand All @@ -35,13 +34,12 @@
return ret;
}
case AF_INET6 : {
ret.resize(18);
ret[0] = ((struct sockaddr_in6 *) addr)->sin6_port >> 8;
ret[1] = ((struct sockaddr_in6 *) addr)->sin6_port & 0xFF;
memcpy(&ret[2], &(((struct sockaddr_in6 *)addr)->sin6_addr), 16);
return ret;
}
default: assert(0); return "";
default: assert(0); return ret;
}
}

Expand Down Expand Up @@ -78,7 +76,7 @@
//主server才创建session map，其他cloned server共享之 [AUTO-TRANSLATED:113cf4fd]
//Only the main server creates a session map, other cloned servers share it
_session_mutex = std::make_shared<std::recursive_mutex>();
_session_map = std::make_shared<std::unordered_map<PeerIdType, SessionHelper::Ptr> >();
_session_map = std::make_shared<SessionMapType>();

// 新建一个定时器定时管理这些 udp 会话,这些对象只由主server做超时管理，cloned server不管理 [AUTO-TRANSLATED:d20478a2]
//Create a timer to manage these udp sessions periodically, these objects are only managed by the main server, cloned servers do not manage them
Expand Down Expand Up @@ -207,7 +205,7 @@
std::lock_guard<std::recursive_mutex> lock(*_session_mutex);
//拷贝map，防止遍历时移除对象 [AUTO-TRANSLATED:ebbc7595]
//Copy the map to prevent objects from being removed during traversal
copy_map = std::make_shared<std::unordered_map<PeerIdType, SessionHelper::Ptr> >(*_session_map);
copy_map = std::make_shared<SessionMapType>(*_session_map);
}
auto lam = [copy_map]() {
for (auto &pr : *copy_map) {
Expand Down Expand Up @@ -277,7 +275,7 @@

assert(_socket);
socket->bindUdpSock(_socket->get_local_port(), _socket->get_local_ip());
socket->bindPeerAddr((struct sockaddr *) addr_str.data(), addr_str.size());

Check warning on line 278 in src/Network/UdpServer.cpp

View workflow job for this annotation

GitHub Actions / build 

 'argument': conversion from 'size_t' to 'int', possible loss of data

auto helper = _session_alloc(server, socket);
// 把本服务器的配置传递给 Session [AUTO-TRANSLATED:e3ed95ab]
Expand Down
42 changes: 40 additions & 2 deletions src/Network/UdpServer.h
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review for src/Network/UdpServer.h:

Code Review: Patch to UdpServer.h

Summary

This patch modifies the UdpServer class to improve its handling of peer IDs, leveraging C++17 features for better performance and type safety. It's a patch review focusing on the changes introduced.

Detailed Feedback

Code Overview

The patch introduces a PeerIdType class, using std::array for C++17-compatible systems and std::string otherwise. It also adds a custom hash function for PeerIdType to improve the performance of the _session_map. The _session_map itself is updated to use the new PeerIdType and the custom hash function.

Strengths

  • Improved Type Safety: Using std::array for PeerIdType provides better type safety compared to std::string when C++17 is available.
  • Potential Performance Improvement: The custom hash function for PeerIdType should improve the performance of lookups in the _session_map, especially with a large number of sessions.
  • C++17 Compatibility: The code gracefully handles both C++17 and pre-C++17 environments.
  • Clearer Code Structure: The use of std::array and the custom hash function makes the code more readable and maintainable.

Areas for Improvement

1. PeerIdType Comparison

  • Issue: The operator== in PeerIdType performs a piecemeal comparison of the underlying data as uint64_t and uint16_t. This is less efficient and less readable than a direct memory comparison. It also assumes a specific memory layout which might be compiler-dependent.
  • Suggestion: Use std::memcmp for a more efficient and portable comparison. This avoids potential issues related to endianness and compiler-specific optimizations.
  • Example:
bool operator==(const PeerIdType &that) const {
    return std::memcmp(data(), that.data(), size()) == 0;
}

2. Error Handling in PeerIdType Constructor

  • Issue: The constructor of PeerIdType truncates the input length if it exceeds the array size, but it doesn't provide any indication of this truncation.
  • Suggestion: Consider adding a check and throwing an exception or returning an error code if the input length is invalid. This would make the code more robust.

3. Documentation

  • Issue: The comments in the code are mostly in English and Chinese. While the Chinese comments might be helpful for some developers, it's generally better to stick to a single language for better maintainability and internationalization.
  • Suggestion: Remove the Chinese comments and improve the English comments to be more concise and informative. Focus on explaining the why behind the code, not just the what.

4. PeerIdHash

  • Issue: The PeerIdHash uses std::string_view which might involve unnecessary copying.
  • Suggestion: If performance is critical, consider using a custom hash function that directly operates on the std::array data without creating a temporary std::string_view. This would avoid the overhead of constructing a std::string_view.

Conclusion

This patch is a significant improvement to the UdpServer class, enhancing type safety and potentially improving performance. Addressing the suggested improvements in PeerIdType comparison, error handling, documentation, and the custom hash function would further enhance the code's robustness, readability, and efficiency. The overall approach is well-structured and demonstrates good understanding of C++17 features.

TRANS_BY_GITHUB_AI_ASSISTANT

Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,45 @@
#ifndef TOOLKIT_NETWORK_UDPSERVER_H
#define TOOLKIT_NETWORK_UDPSERVER_H

#if __cplusplus >= 201703L
#include <array>
#include <string_view>
#endif
#include "Server.h"
#include "Session.h"

#define PEERTYPELEN (18)
namespace toolkit {

class UdpServer : public Server {
public:
using Ptr = std::shared_ptr<UdpServer>;
#if __cplusplus >= 201703L
class PeerIdType : public std::array<char, PEERTYPELEN> {
public:
PeerIdType() {
fill(' ');
}
PeerIdType(size_t len, char fillChar) {
if (len > size()) {
len = size();
Copy link
Member

@xia-chu xia-chu Dec 6, 2024 •
edited by alexliyu7352
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing the PeerIdType(size_t len, char fillChar) constructor,
In the case of C++11, let PeerIdType inherit from std::string,
In its default constructor, call resize(18) directly.

我建议删除PeerIdType(size_t len, char fillChar) 这个构造函数，
在c++11的情况下，让PeerIdType派生于std::string即可，
在他的默认构造函数中调用下resize(18)即可

TRANS_BY_GITHUB_AI_ASSISTANT

}
fill(fillChar);
}
bool operator==(const PeerIdType &that) const {
return (as<uint64_t>(0) == that.as<uint64_t>(0)) && (as<uint64_t>(8) == that.as<uint64_t>(8)) && (as<uint16_t>(16) == that.as<uint16_t>(16));
}

private:
template <class T>
const T& as(size_t offset) const {
return *(reinterpret_cast<const T *>(data() + offset));
}
};
#else
using PeerIdType = std::string;
#endif

using Ptr = std::shared_ptr<UdpServer>;
using onCreateSocket = std::function<Socket::Ptr(const EventPoller::Ptr &, const Buffer::Ptr &, struct sockaddr *, int)>;

explicit UdpServer(const EventPoller::Ptr &poller = nullptr);
Expand Down Expand Up @@ -75,6 +105,14 @@ class UdpServer : public Server {
virtual void cloneFrom(const UdpServer &that);

private:
#if __cplusplus >= 201703L
struct PeerIdHash {
size_t operator()(const PeerIdType &v) const noexcept { return std::hash<std::string_view> {}(std::string_view(v.data(), v.size())); }
};
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr, PeerIdHash>;
#else
using SessionMapType = std::unordered_map<PeerIdType, SessionHelper::Ptr>;
#endif
/**
* @brief 开始udp server
* @param port 本机端口，0则随机
Expand Down Expand Up @@ -150,7 +188,7 @@ class UdpServer : public Server {
//cloned server共享主server的session map，防止数据在不同server间漂移 [AUTO-TRANSLATED:9a149e52]
//Cloned server shares the session map with the main server, preventing data drift between different servers
std::shared_ptr<std::recursive_mutex> _session_mutex;
std::shared_ptr<std::unordered_map<PeerIdType, SessionHelper::Ptr> > _session_map;
std::shared_ptr<SessionMapType> _session_map;
//主server持有cloned server的引用 [AUTO-TRANSLATED:04a6403a]
//Main server holds a reference to the cloned server
std::unordered_map<EventPoller *, Ptr> _cloned_server;
Expand Down
Loading