Skip to content

Fix getaddrinfo error reporting #278

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

Merged
Enmk merged 4 commits into from
Jan 19, 2023
+49 −0
Merged

Fix getaddrinfo error reporting #278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2596887
Fix getaddrinfo error reporting
nUl1 Jan 5, 2023
bda1c11
typo
nUl1 Jan 5, 2023
a664331
fix test
nUl1 Jan 5, 2023
217d7eb
Merge branch 'master' into fix-gaierror
Enmk Jan 18, 2023
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
21 changes: 21 additions & 0 deletions clickhouse/base/socket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,21 @@ windowsErrorCategory const& windowsErrorCategory::category() {
}
#endif

#if defined(_unix_)
char const* getaddrinfoErrorCategory::name() const noexcept {
return "getaddrinfoError";
}

std::string getaddrinfoErrorCategory::message(int c) const {
return gai_strerror(c);
}

getaddrinfoErrorCategory const& getaddrinfoErrorCategory::category() {
static getaddrinfoErrorCategory c;
return c;
}
#endif

namespace {

class LocalNames : public std::unordered_set<std::string> {
Expand Down Expand Up @@ -265,6 +280,12 @@ NetworkAddress::NetworkAddress(const std::string& host, const std::string& port)

const int error = getaddrinfo(host.c_str(), port.c_str(), &hints, &info_);

#if defined(_unix_)
if (error && error != EAI_SYSTEM) {
throw std::system_error(error, getaddrinfoErrorCategory::category());
}
#endif

if (error) {
throw std::system_error(getSocketErrorCode(), getErrorCategory());
}
Expand Down
12 changes: 12 additions & 0 deletions clickhouse/base/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,18 @@ class windowsErrorCategory : public std::error_category {

#endif

#if defined(_unix_)

class getaddrinfoErrorCategory : public std::error_category {

@Enmk Enmk Jan 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you have to make it part of the API, i.e. visible to the users? IMO it would be better to move declaration to .cpp

@nUl1 nUl1 Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's no requirement for it to be public. But users will have access to it through e.code().category() and nothing to compare it to.

public:
char const* name() const noexcept override final;
std::string message(int c) const override final;

static getaddrinfoErrorCategory const& category();
};

#endif


class SocketBase {
public:
Expand Down
16 changes: 16 additions & 0 deletions ut/socket_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
#include <string.h>
#include <thread>

// for EAI_* error codes
#if defined(_win_)
# include <ws2tcpip.h>
#else
# include <netdb.h>
#endif

using namespace clickhouse;

TEST(Socketcase, connecterror) {
Expand Down Expand Up @@ -63,6 +70,15 @@ TEST(Socketcase, timeoutrecv) {
server.stop();
}

TEST(Socketcase, gaierror) {
try {
NetworkAddress addr("host.invalid", "80"); // never resolves
FAIL();
} catch (const std::system_error& e) {
ASSERT_PRED1([](int error) { return error == EAI_NONAME || error == EAI_AGAIN || error == EAI_FAIL; }, e.code().value());
}
}

TEST(Socketcase, connecttimeout) {
using Clock = std::chrono::steady_clock;

Expand Down