You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/12/16 17:18:14 UTC
[GitHub] [incubator-kvrocks] git-hulk opened a new pull request, #1183: Fix SockConect doesn't resolve domain before connecting
git-hulk opened a new pull request, #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183
Currently, Kvrocks supports connecting the server with timeout or not, but the behavior is a bit different between them. The timeout version doesn't support resolving the domain before connecting to the server, so it'll be broken if it connects with the timeout.
SockConnect with timeout is the first time to use in #1172, so this bug won't affect the previous releases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#discussion_r1051304229
##########
src/common/io_util.cc:
##########
@@ -131,57 +97,67 @@ Status SockSetTcpKeepalive(int fd, int interval) {
}
Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_timeout, int timeout) {
- if (conn_timeout == 0) {
- auto s = SockConnect(host, port, fd);
- if (!s) return s;
- } else {
- *fd = socket(AF_INET, SOCK_STREAM, 0);
- if (*fd == NullFD) return Status::FromErrno();
- }
-
- auto exit = MakeScopeExit([fd] {
- close(*fd);
- *fd = NullFD;
- });
+ addrinfo hints, *servinfo = nullptr, *p = nullptr;
- if (conn_timeout != 0) {
- sockaddr_in sin{};
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = inet_addr(host.c_str());
- sin.sin_port = htons(port);
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
- fcntl(*fd, F_SETFL, O_NONBLOCK);
- connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
Review Comment:
Yes, it's duplicated code. I will revise this PR again and add a test case for this issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#discussion_r1051380263
##########
src/common/io_util.cc:
##########
@@ -131,57 +97,66 @@ Status SockSetTcpKeepalive(int fd, int interval) {
}
Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_timeout, int timeout) {
- if (conn_timeout == 0) {
- auto s = SockConnect(host, port, fd);
- if (!s) return s;
- } else {
- *fd = socket(AF_INET, SOCK_STREAM, 0);
- if (*fd == NullFD) return Status::FromErrno();
- }
+ addrinfo hints, *servinfo = nullptr, *p = nullptr;
- auto exit = MakeScopeExit([fd] {
- close(*fd);
- *fd = NullFD;
- });
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
- if (conn_timeout != 0) {
- sockaddr_in sin{};
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = inet_addr(host.c_str());
- sin.sin_port = htons(port);
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
+ auto exit = MakeScopeExit([servinfo] { freeaddrinfo(servinfo); });
- fcntl(*fd, F_SETFL, O_NONBLOCK);
- connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
+ for (p = servinfo; p != nullptr; p = p->ai_next) {
+ auto cfd = UniqueFD(socket(p->ai_family, p->ai_socktype, p->ai_protocol));
+ if (!cfd) continue;
- auto retmask = Util::aeWait(*fd, AE_WRITABLE, conn_timeout);
- if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
- return Status::FromErrno();
+ if (conn_timeout == 0) {
+ if (connect(*cfd, p->ai_addr, p->ai_addrlen) == -1) {
+ continue;
+ }
+ } else {
+ fcntl(*cfd, F_SETFL, O_NONBLOCK);
+ int ret = connect(*cfd, p->ai_addr, p->ai_addrlen);
+ if (ret != 0 && errno != EINPROGRESS) {
+ continue;
+ }
+ auto retmask = Util::aeWait(*cfd, AE_WRITABLE, conn_timeout);
+ if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
+ return Status::FromErrno();
+ }
+
+ // restore to the block mode
+ int socket_arg = 0;
+ if ((socket_arg = fcntl(*cfd, F_GETFL, NULL)) < 0) {
+ return Status::FromErrno();
+ }
+ socket_arg &= (~O_NONBLOCK);
+ if (fcntl(*cfd, F_SETFL, socket_arg) < 0) {
+ return Status::FromErrno();
+ }
}
- int socket_arg = 0;
- // Set to blocking mode again...
- if ((socket_arg = fcntl(*fd, F_GETFL, NULL)) < 0) {
- return Status::FromErrno();
+ Status s = SockSetTcpKeepalive(*cfd, 120);
+ if (s.IsOK()) {
+ s = SockSetTcpNoDelay(*cfd, 1);
}
- socket_arg &= (~O_NONBLOCK);
- if (fcntl(*fd, F_SETFL, socket_arg) < 0) {
- return Status::FromErrno();
+ if (!s.IsOK()) {
+ continue;
Review Comment:
Looks better to continue if it has other addresses to have a try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#discussion_r1051303354
##########
src/common/io_util.cc:
##########
@@ -131,57 +97,67 @@ Status SockSetTcpKeepalive(int fd, int interval) {
}
Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_timeout, int timeout) {
- if (conn_timeout == 0) {
- auto s = SockConnect(host, port, fd);
- if (!s) return s;
- } else {
- *fd = socket(AF_INET, SOCK_STREAM, 0);
- if (*fd == NullFD) return Status::FromErrno();
- }
-
- auto exit = MakeScopeExit([fd] {
- close(*fd);
- *fd = NullFD;
- });
+ addrinfo hints, *servinfo = nullptr, *p = nullptr;
- if (conn_timeout != 0) {
- sockaddr_in sin{};
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = inet_addr(host.c_str());
- sin.sin_port = htons(port);
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
- fcntl(*fd, F_SETFL, O_NONBLOCK);
- connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
Review Comment:
Duplicated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#issuecomment-1356157452
Thanks all, merging...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#discussion_r1051329162
##########
src/common/io_util.cc:
##########
@@ -131,57 +97,66 @@ Status SockSetTcpKeepalive(int fd, int interval) {
}
Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_timeout, int timeout) {
- if (conn_timeout == 0) {
- auto s = SockConnect(host, port, fd);
- if (!s) return s;
- } else {
- *fd = socket(AF_INET, SOCK_STREAM, 0);
- if (*fd == NullFD) return Status::FromErrno();
- }
+ addrinfo hints, *servinfo = nullptr, *p = nullptr;
- auto exit = MakeScopeExit([fd] {
- close(*fd);
- *fd = NullFD;
- });
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
- if (conn_timeout != 0) {
- sockaddr_in sin{};
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = inet_addr(host.c_str());
- sin.sin_port = htons(port);
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
+ auto exit = MakeScopeExit([servinfo] { freeaddrinfo(servinfo); });
- fcntl(*fd, F_SETFL, O_NONBLOCK);
- connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
+ for (p = servinfo; p != nullptr; p = p->ai_next) {
+ auto cfd = UniqueFD(socket(p->ai_family, p->ai_socktype, p->ai_protocol));
+ if (!cfd) continue;
- auto retmask = Util::aeWait(*fd, AE_WRITABLE, conn_timeout);
- if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
- return Status::FromErrno();
+ if (conn_timeout == 0) {
+ if (connect(*cfd, p->ai_addr, p->ai_addrlen) == -1) {
+ continue;
+ }
+ } else {
+ fcntl(*cfd, F_SETFL, O_NONBLOCK);
+ int ret = connect(*cfd, p->ai_addr, p->ai_addrlen);
+ if (ret != 0 && errno != EINPROGRESS) {
+ continue;
+ }
+ auto retmask = Util::aeWait(*cfd, AE_WRITABLE, conn_timeout);
+ if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
+ return Status::FromErrno();
+ }
+
+ // restore to the block mode
+ int socket_arg = 0;
+ if ((socket_arg = fcntl(*cfd, F_GETFL, NULL)) < 0) {
+ return Status::FromErrno();
+ }
+ socket_arg &= (~O_NONBLOCK);
+ if (fcntl(*cfd, F_SETFL, socket_arg) < 0) {
+ return Status::FromErrno();
+ }
}
- int socket_arg = 0;
- // Set to blocking mode again...
- if ((socket_arg = fcntl(*fd, F_GETFL, NULL)) < 0) {
- return Status::FromErrno();
+ Status s = SockSetTcpKeepalive(*cfd, 120);
+ if (s.IsOK()) {
+ s = SockSetTcpNoDelay(*cfd, 1);
}
- socket_arg &= (~O_NONBLOCK);
- if (fcntl(*fd, F_SETFL, socket_arg) < 0) {
- return Status::FromErrno();
+ if (!s.IsOK()) {
+ continue;
Review Comment:
continue or return error directly, which is better here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on a diff in pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183#discussion_r1051328711
##########
src/common/io_util.cc:
##########
@@ -131,57 +97,66 @@ Status SockSetTcpKeepalive(int fd, int interval) {
}
Status SockConnect(const std::string &host, uint32_t port, int *fd, int conn_timeout, int timeout) {
- if (conn_timeout == 0) {
- auto s = SockConnect(host, port, fd);
- if (!s) return s;
- } else {
- *fd = socket(AF_INET, SOCK_STREAM, 0);
- if (*fd == NullFD) return Status::FromErrno();
- }
+ addrinfo hints, *servinfo = nullptr, *p = nullptr;
- auto exit = MakeScopeExit([fd] {
- close(*fd);
- *fd = NullFD;
- });
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
- if (conn_timeout != 0) {
- sockaddr_in sin{};
- sin.sin_family = AF_INET;
- sin.sin_addr.s_addr = inet_addr(host.c_str());
- sin.sin_port = htons(port);
+ if (int rv = getaddrinfo(host.c_str(), std::to_string(port).c_str(), &hints, &servinfo); rv != 0) {
+ return {Status::NotOK, gai_strerror(rv)};
+ }
+ auto exit = MakeScopeExit([servinfo] { freeaddrinfo(servinfo); });
- fcntl(*fd, F_SETFL, O_NONBLOCK);
- connect(*fd, reinterpret_cast<sockaddr *>(&sin), sizeof(sin));
+ for (p = servinfo; p != nullptr; p = p->ai_next) {
+ auto cfd = UniqueFD(socket(p->ai_family, p->ai_socktype, p->ai_protocol));
+ if (!cfd) continue;
- auto retmask = Util::aeWait(*fd, AE_WRITABLE, conn_timeout);
- if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
- return Status::FromErrno();
+ if (conn_timeout == 0) {
+ if (connect(*cfd, p->ai_addr, p->ai_addrlen) == -1) {
+ continue;
+ }
+ } else {
+ fcntl(*cfd, F_SETFL, O_NONBLOCK);
+ int ret = connect(*cfd, p->ai_addr, p->ai_addrlen);
+ if (ret != 0 && errno != EINPROGRESS) {
+ continue;
+ }
+ auto retmask = Util::aeWait(*cfd, AE_WRITABLE, conn_timeout);
+ if ((retmask & AE_WRITABLE) == 0 || (retmask & AE_ERROR) != 0 || (retmask & AE_HUP) != 0) {
+ return Status::FromErrno();
+ }
+
+ // restore to the block mode
+ int socket_arg = 0;
+ if ((socket_arg = fcntl(*cfd, F_GETFL, NULL)) < 0) {
+ return Status::FromErrno();
+ }
+ socket_arg &= (~O_NONBLOCK);
+ if (fcntl(*cfd, F_SETFL, socket_arg) < 0) {
+ return Status::FromErrno();
+ }
}
- int socket_arg = 0;
- // Set to blocking mode again...
- if ((socket_arg = fcntl(*fd, F_GETFL, NULL)) < 0) {
- return Status::FromErrno();
+ Status s = SockSetTcpKeepalive(*cfd, 120);
+ if (s.IsOK()) {
+ s = SockSetTcpNoDelay(*cfd, 1);
}
- socket_arg &= (~O_NONBLOCK);
- if (fcntl(*fd, F_SETFL, socket_arg) < 0) {
- return Status::FromErrno();
+ if (!s.IsOK()) {
Review Comment:
```suggestion
else {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1183: Fix SockConect doesn't resolve domain before connecting
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #1183:
URL: https://github.com/apache/incubator-kvrocks/pull/1183
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org