Copilot commented on code in PR #7001:
URL: https://github.com/apache/ignite-3/pull/7001#discussion_r2538499107
##########
modules/platforms/cpp/ignite/odbc/config/configuration.cpp:
##########
@@ -125,6 +128,16 @@ void configuration::from_config_map(const config_map
&config_params) {
try_get_string_param(m_ssl_key_file, config_params, key::ssl_key_file);
try_get_string_param(m_ssl_cert_file, config_params, key::ssl_cert_file);
try_get_string_param(m_ssl_ca_file, config_params, key::ssl_ca_file);
+
+ auto heartbeat_interval_it = config_params.find(key::heartbeat_interval);
+ if (heartbeat_interval_it != config_params.end()) {
+ auto heartbeat_interval_opt =
parse_int<std::int32_t>(heartbeat_interval_it->second);
+ if (!heartbeat_interval_opt)
+ throw
odbc_error(sql_state::S01S00_INVALID_CONNECTION_STRING_ATTRIBUTE,
+ "Invalid page size value: " + heartbeat_interval_it->second);
Review Comment:
Incorrect error message. The error message "Invalid page size value:" should
be "Invalid heartbeat interval value:" to correctly describe what is being
validated.
```suggestion
"Invalid heartbeat interval value: " +
heartbeat_interval_it->second);
```
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -32,16 +39,24 @@ void sql_environment::deregister_connection(sql_connection
*conn) {
m_connections.erase(conn);
}
-sql_result sql_environment::internal_create_connection(sql_connection *&conn) {
- conn = new sql_connection(this);
+sql_result sql_environment::internal_create_connection(sql_connection_ptr
*&conn) {
+ auto conn_raw = new(std::nothrow) sql_connection(this, m_timer_thread);
+ if (!conn_raw) {
+ add_status_record(sql_state::SHY001_MEMORY_ALLOCATION, "Not enough
memory.");
+ return sql_result::AI_ERROR;
+ }
+
+ conn = new(std::nothrow) sql_connection_ptr(conn_raw);
if (!conn) {
+ delete conn;
Review Comment:
Memory leak: When the second allocation fails, the code attempts to `delete
conn;` but `conn` is the pointer that just failed to allocate and is likely
null or invalid. Instead, `conn_raw` should be deleted here to avoid leaking
the first allocation.
```suggestion
delete conn_raw;
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -802,4 +822,38 @@ void sql_connection::on_observable_timestamp(std::int64_t
timestamp) {
}
}
+void sql_connection::send_heartbeat() {
+ network::data_buffer_owning response;
+ auto success = catch_errors([&] {
+ response = sync_request(protocol::client_operation::HEARTBEAT,
[&](protocol::writer &) {});
+ });
+
+ UNUSED_VALUE(response);
+
+ if (success)
+ plan_heartbeat(m_heartbeat_interval);
Review Comment:
Potential issue with heartbeat rescheduling. If `send_heartbeat()` fails
(when `catch_errors` returns false), the heartbeat is not rescheduled, which
means heartbeats will stop entirely. Consider rescheduling the heartbeat even
on failure, potentially with a backoff strategy, to ensure the connection
remains monitored.
```suggestion
// Always reschedule the heartbeat, even if sending failed.
plan_heartbeat(m_heartbeat_interval);
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.h:
##########
@@ -40,9 +41,12 @@ class sql_statement;
/**
* ODBC node connection.
*/
-class sql_connection : public diagnosable_adapter {
+class sql_connection : public diagnosable_adapter, public
std::enable_shared_from_this<sql_connection> {
Review Comment:
Extra whitespace in the `enable_shared_from_this` declaration. There's a
double space before the opening brace: `enable_shared_from_this<sql_connection>
{` should be `enable_shared_from_this<sql_connection> {`.
```suggestion
class sql_connection : public diagnosable_adapter, public
std::enable_shared_from_this<sql_connection> {
```
##########
modules/platforms/cpp/ignite/odbc/sql_connection.cpp:
##########
@@ -70,6 +70,10 @@ std::vector<ignite::end_point> collect_addresses(const
ignite::configuration &cf
namespace ignite {
+sql_connection::~sql_connection() {
Review Comment:
Potential race condition in destructor. The destructor calls `deregister()`
which modifies the environment's connection set. If heartbeat callbacks are
still scheduled or executing when the destructor runs, they could attempt to
access the connection after it's been deregistered or partially destroyed.
Consider canceling any pending heartbeat timers in the destructor before
calling `deregister()`.
```suggestion
void sql_connection::cancel_heartbeat() {
// TODO: Implement heartbeat timer cancellation logic here.
}
sql_connection::~sql_connection() {
cancel_heartbeat();
cancel_heartbeat();
```
##########
modules/platforms/cpp/ignite/odbc/sql_environment.cpp:
##########
@@ -20,8 +20,15 @@
namespace ignite {
-sql_connection *sql_environment::create_connection() {
- sql_connection *connection;
+sql_environment::sql_environment()
+ : m_timer_thread(detail::thread_timer::start([&] (auto&& err) {
+ std::cout << "[SQL ENV CTOR] error: " << err.what() << std::endl;
Review Comment:
Debug output left in production code. The line `std::cout << "[SQL ENV CTOR]
error: " << err.what() << std::endl;` should be removed or replaced with proper
logging.
```suggestion
```
--
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]
