Copilot commented on code in PR #7478: URL: https://github.com/apache/ignite-3/pull/7478#discussion_r2735590928
########## modules/platforms/cpp/tests/fake_server/socket_adapter/win/client_socket_adapter.h: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <winsock2.h> + +namespace ignite { +class client_socket_adapter { +public: + explicit client_socket_adapter(SOCKET m_fd) + : m_fd(m_fd) {} + + client_socket_adapter() = default; + + client_socket_adapter(const client_socket_adapter &other) = default; + + client_socket_adapter &operator=(const client_socket_adapter &other) = default; + + bool is_valid() const { return m_fd != INVALID_SOCKET; } + + void send_message(const std::vector<std::byte> &msg) { + ::send(m_fd, reinterpret_cast<const char *>(msg.data()), msg.size(), 0); + } + + int recieve_next_packet(std::byte *buf, size_t buf_size) { + return ::recv(m_fd, reinterpret_cast<char *>(buf), buf_size, 0); Review Comment: The Windows `client_socket_adapter` header uses `std::vector<std::byte>` and `size_t` but only includes `<winsock2.h>`, without `<vector>` or `<cstddef>`. To keep headers self‑contained and avoid relying on indirect standard library includes, please add the appropriate standard headers here as well. ########## modules/platforms/cpp/tests/fake_server/socket_adapter/posix/server_socket_adapter.h: ########## @@ -0,0 +1,73 @@ +/* +* Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <netinet/in.h> +#include <unistd.h> + +namespace ignite { +class server_socket_adapter { +public: + explicit server_socket_adapter(int m_fd) + : m_fd(m_fd) {} + + server_socket_adapter() = default; + + server_socket_adapter(const server_socket_adapter &other) = default; + + server_socket_adapter &operator=(const server_socket_adapter &other) = default; + + void start() { + m_fd = ::socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + } + + bool is_valid() const { + return m_fd >= 0; + } + + int accept() { + sockaddr_in cl_addr{}; + + socklen_t addr_len = sizeof(cl_addr); + + int cl_sock = ::accept(m_fd, reinterpret_cast<sockaddr *>(&cl_addr), &addr_len); + + return cl_sock; + } + + int bind(int port) const { + sockaddr_in srv_addr{}; + + srv_addr.sin_family = AF_INET; + srv_addr.sin_addr.s_addr = htonl(INADDR_ANY); + srv_addr.sin_port = htons(port); + + return ::bind(m_fd, reinterpret_cast<sockaddr*>(&srv_addr), sizeof(srv_addr)); + } + + int listen() const { + return ::listen(m_fd, 1); Review Comment: `server_socket_adapter::start()` and other methods use POSIX socket APIs (`::socket`, `::accept`, `::bind`, `::listen`), but this header only includes `<netinet/in.h>` and `<unistd.h>`; it does not include `<sys/socket.h>`, which is where these functions are actually declared on POSIX. Please add `<sys/socket.h>` here so that the adapter header provides the correct declarations without depending on previous includes in the translation unit. ########## modules/platforms/cpp/tests/fake_server/tcp_client_channel.cpp: ########## @@ -18,9 +18,10 @@ #include "tcp_client_channel.h" #include <algorithm> -#include <netinet/in.h> -#include <sys/socket.h> -#include <unistd.h> + +#ifndef _WIN32 + +#endif Review Comment: The empty `#ifndef _WIN32` / `#endif` block here has no effect and looks like a leftover from earlier POSIX‑specific includes; keeping unused preprocessor conditionals around makes the file harder to read and can confuse future edits. Please either remove these lines or move any platform‑specific includes or code that still needs to be guarded back inside this conditional. ```suggestion ``` ########## modules/platforms/cpp/tests/fake_server/socket_adapter/win/client_socket_adapter.h: ########## @@ -0,0 +1,51 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <winsock2.h> + +namespace ignite { +class client_socket_adapter { +public: + explicit client_socket_adapter(SOCKET m_fd) + : m_fd(m_fd) {} + + client_socket_adapter() = default; + + client_socket_adapter(const client_socket_adapter &other) = default; + + client_socket_adapter &operator=(const client_socket_adapter &other) = default; + + bool is_valid() const { return m_fd != INVALID_SOCKET; } + + void send_message(const std::vector<std::byte> &msg) { + ::send(m_fd, reinterpret_cast<const char *>(msg.data()), msg.size(), 0); + } + + int recieve_next_packet(std::byte *buf, size_t buf_size) { + return ::recv(m_fd, reinterpret_cast<char *>(buf), buf_size, 0); + } + Review Comment: The method name `recieve_next_packet` is misspelled ("recieve" instead of "receive"), and this typo is now part of the public API used by `tcp_client_channel`. To keep the test code and adapters readable and avoid propagating the typo, consider renaming the method to `receive_next_packet` (providing a temporary compatibility alias if needed). ```suggestion int receive_next_packet(std::byte *buf, size_t buf_size) { return ::recv(m_fd, reinterpret_cast<char *>(buf), buf_size, 0); } // Temporary compatibility alias for the misspelled method name. // Prefer using receive_next_packet in new code. int recieve_next_packet(std::byte *buf, size_t buf_size) { return receive_next_packet(buf, buf_size); } ``` ########## modules/platforms/cpp/tests/fake_server/socket_adapter/posix/client_socket_adapter.h: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <sys/socket.h> +#include <unistd.h> + +namespace ignite { +class client_socket_adapter { +public: + explicit client_socket_adapter(int m_fd) + : m_fd(m_fd) {} + + client_socket_adapter() = default; + + client_socket_adapter(const client_socket_adapter &other) = default; + + client_socket_adapter &operator=(const client_socket_adapter &other) = default; + + bool is_valid() const { return m_fd >= 0; } + + void send_message(const std::vector<std::byte> &msg) { ::send(m_fd, msg.data(), msg.size(), 0); } + + int recieve_next_packet(std::byte *buf, size_t buf_size) { return ::recv(m_fd, buf, buf_size, 0); } + + void close() { Review Comment: Both the POSIX and Windows `client_socket_adapter` headers use `std::vector<std::byte>` and `size_t` in method signatures, but neither of them includes `<vector>` or `<cstddef>`. This makes these headers non self‑contained and can lead to build failures or brittle dependencies on indirect includes; please add the necessary standard headers explicitly so that each adapter header can be included on its own. ########## modules/platforms/cpp/tests/fake_server/fake_server.h: ########## @@ -43,19 +42,33 @@ class fake_server { : m_srv_port(srv_port) , m_logger(std::move(logger)) , m_op_type_handler(op_type_handler) - {} + { +#ifdef _WIN32 + static bool wsa_initialized = false; + if (!wsa_initialized) { + WSADATA wsaData; + if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0) { + throw ignite_error("WSAStartup failed"); + } + wsa_initialized = true; + } +#endif Review Comment: On Windows the Winsock initialization/cleanup logic here is unbalanced: `WSAStartup` is guarded by a `static bool wsa_initialized` flag, but `WSACleanup()` in the destructor is called unconditionally for every `fake_server` instance. This means that after the first instance is destroyed Winsock will be cleaned up, and subsequent `fake_server` instances will never re‑initialize it (because `wsa_initialized` stays `true`), leading to undefined behavior or failures when using sockets. Consider either (a) using a reference count or a separate process‑wide initializer that pairs `WSAStartup` and `WSACleanup`, or (b) delegating to the existing `ignite::network::detail::init_wsa()` helper so that test code does not own Winsock lifecycle at all. ########## modules/platforms/cpp/tests/fake_server/socket_adapter/posix/client_socket_adapter.h: ########## @@ -0,0 +1,48 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once +#include <sys/socket.h> +#include <unistd.h> + +namespace ignite { +class client_socket_adapter { +public: + explicit client_socket_adapter(int m_fd) + : m_fd(m_fd) {} + + client_socket_adapter() = default; + + client_socket_adapter(const client_socket_adapter &other) = default; + + client_socket_adapter &operator=(const client_socket_adapter &other) = default; + + bool is_valid() const { return m_fd >= 0; } + + void send_message(const std::vector<std::byte> &msg) { ::send(m_fd, msg.data(), msg.size(), 0); } + + int recieve_next_packet(std::byte *buf, size_t buf_size) { return ::recv(m_fd, buf, buf_size, 0); } Review Comment: The method name `recieve_next_packet` is misspelled ("recieve" instead of "receive"), and this typo is now part of the adapter interface used by `tcp_client_channel`. To keep the code consistent and readable, please rename this to `receive_next_packet` (and adjust call sites accordingly). ```suggestion int receive_next_packet(std::byte *buf, size_t buf_size) { return ::recv(m_fd, buf, buf_size, 0); } ``` ########## modules/platforms/cpp/tests/fake_server/tcp_client_channel.h: ########## @@ -24,28 +24,41 @@ #include <ignite/common/ignite_error.h> #include <ignite/protocol/utils.h> +#ifdef _WIN32 +#include "socket_adapter/win/server_socket_adapter.h" +#include "socket_adapter/win/client_socket_adapter.h" + +#define LAST_SOCKET_ERROR() WSAGetLastError() +#else +#include "socket_adapter/posix/server_socket_adapter.h" +#include "socket_adapter/posix/client_socket_adapter.h" Review Comment: The helper macro `LAST_SOCKET_ERROR()` expands to `strerror(errno)` on POSIX, but this header does not include `<cerrno>` or `<cstring>`, nor do the directly included headers, so `errno`/`strerror` will be undeclared on some compilers and configurations. To make this header self‑contained and avoid relying on transitive system includes, please add the appropriate standard headers (for example `<cerrno>` and `<cstring>`) in the non‑Windows branch. ```suggestion #include "socket_adapter/posix/client_socket_adapter.h" #include <cerrno> #include <cstring> ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
