Copilot commented on code in PR #7528:
URL: https://github.com/apache/ignite-3/pull/7528#discussion_r2814979589
##########
modules/platforms/python/cpp_module/py_connection.cpp:
##########
@@ -194,21 +194,32 @@ int register_py_connection_type(PyObject* mod) {
}
PyObject *make_py_connection(std::vector<ignite::end_point> addresses, const
char* schema, const char* identity,
- const char* secret, int page_size, int timeout, bool autocommit,
ssl_config &&ssl_cfg) {
+ const char* secret, int page_size, int timeout, double heartbeat_interval,
bool autocommit, ssl_config &&ssl_cfg) {
if (addresses.empty()) {
PyErr_SetString(py_get_module_interface_error_class(), "No addresses
provided to connect");
return nullptr;
}
Review Comment:
Missing input validation: The heartbeat_interval parameter is not validated
for negative values. A negative value would result in undefined behavior when
converted to std::chrono::milliseconds and could cause issues with the timer.
Consider adding validation to ensure heartbeat_interval is non-negative, e.g.,
`if (heartbeat_interval < 0) { PyErr_SetString(...); return nullptr; }`.
```suggestion
if (heartbeat_interval < 0) {
PyErr_SetString(py_get_module_interface_error_class(),
"heartbeat_interval must be non-negative");
return nullptr;
}
```
##########
modules/platforms/python/cpp_module/py_connection.cpp:
##########
@@ -66,7 +66,7 @@ PyObject* py_connection_close(py_connection* self, PyObject*)
PyObject* py_connection_cursor(py_connection* self, PyObject*)
{
if (self->m_connection) {
- std::unique_ptr<statement> stmt =
std::make_unique<statement>(*self->m_connection);
+ std::unique_ptr<statement> stmt =
std::make_unique<statement>(**self->m_connection);
Review Comment:
Critical lifetime issue: The statement is being created with a reference to
the node_connection (`**self->m_connection`), but the connection is now managed
via shared_ptr for heartbeat functionality. If the Python connection object is
closed while a cursor still exists, the shared_ptr will be deleted (line 59),
leaving the statement with a dangling reference to the deleted connection. This
can lead to use-after-free bugs. The statement should either hold a shared_ptr
to keep the connection alive, or the cursor's lifetime should be strictly tied
to the connection's Python object lifetime.
##########
modules/platforms/python/cpp_module/module.cpp:
##########
@@ -96,15 +99,16 @@ PyObject* pyignite_dbapi_connect(PyObject*, PyObject* args,
PyObject* kwargs) {
const char *schema = nullptr;
const char *timezone = nullptr;
int timeout = 0;
+ double heartbeat_interval = .0;
Review Comment:
Default value mismatch: The heartbeat_interval is initialized to 0.0 when
not provided by the user, but the documentation states the default should be 30
seconds. This means heartbeats will be disabled by default, contrary to what's
documented. Consider changing the default initialization to 30.0 to match the
documentation, or update the C++ code to check if the value is 0 and use
DEFAULT_HEARTBEAT_INTERVAL in that case.
```suggestion
double heartbeat_interval = 30.0;
```
##########
modules/platforms/python/cpp_module/node_connection.h:
##########
@@ -652,6 +689,15 @@ class node_connection final {
/** Observable timestamp. */
std::atomic_int64_t m_observable_timestamp{0};
- /** SSL Configuration. */
- const ssl_config m_ssl_config;
+ /** Heartbeat interval. */
+ std::chrono::milliseconds m_heartbeat_interval{0};
+
+ /** Last message timestamp. */
+ std::chrono::steady_clock::time_point m_last_message_ts{};
Review Comment:
Potential data race: m_last_message_ts is accessed from both the main thread
(during send_all at line 273) and the timer thread (in on_heartbeat_timeout at
line 647) without synchronization. While this might work in practice because
time_point reads/writes are likely atomic on most platforms, it's technically
undefined behavior according to the C++ memory model. Consider using
std::atomic<std::chrono::steady_clock::time_point> or protecting access with
the socket_mutex.
```suggestion
std::atomic<std::chrono::steady_clock::time_point> m_last_message_ts{};
```
##########
modules/platforms/python/cpp_module/py_connection.cpp:
##########
@@ -194,21 +194,32 @@ int register_py_connection_type(PyObject* mod) {
}
PyObject *make_py_connection(std::vector<ignite::end_point> addresses, const
char* schema, const char* identity,
- const char* secret, int page_size, int timeout, bool autocommit,
ssl_config &&ssl_cfg) {
+ const char* secret, int page_size, int timeout, double heartbeat_interval,
bool autocommit, ssl_config &&ssl_cfg) {
if (addresses.empty()) {
PyErr_SetString(py_get_module_interface_error_class(), "No addresses
provided to connect");
return nullptr;
}
- auto node_connection = std::make_unique<class node_connection>(
- addresses,
- schema ? schema : "",
- identity ? identity : "",
- secret ? secret : "",
- page_size ? page_size : 1024,
- timeout,
- autocommit,
- std::move(ssl_cfg));
+ auto heartbeat_interval_chrono =
std::chrono::milliseconds(static_cast<int>(std::ceilf(heartbeat_interval *
1000)));
+ node_connection::configuration cfg{addresses, autocommit, ssl_cfg,
heartbeat_interval_chrono};
+
+ if (schema)
+ cfg.m_schema = schema;
+
+ if (identity)
+ cfg.m_auth_configuration.m_identity = identity;
+
+ if (secret)
+ cfg.m_auth_configuration.m_secret = secret;
+
+
Review Comment:
Extra blank line - this appears to be unintentional. Consider removing the
duplicate blank line for consistency with the rest of the code.
```suggestion
```
##########
modules/platforms/python/tests/test_connect.py:
##########
@@ -57,3 +59,43 @@ def test_connection_wrong_arg(address, err_msg):
with pytest.raises(pyignite_dbapi.InterfaceError) as err:
pyignite_dbapi.connect(address=address, timeout=1)
assert err.match(err_msg)
+
+
[email protected]("interval", [2.0, 20.0, 0.0001])
+def test_heartbeat_enabled(table_name, drop_table_cleanup, interval):
+ row_count = 10
+ with pyignite_dbapi.connect(address=server_addresses_basic[0],
heartbeat_interval=interval) as conn:
+ with conn.cursor() as cursor:
+ cursor.execute(f"create table {table_name}(id int primary key,
data varchar)")
+ for key in range(row_count):
+ cursor.execute(f"insert into {table_name} values({key},
'data-{key*2}')")
+ assert cursor.rowcount == 1
+
+ data_out = {}
+ for key in range(row_count):
+ cursor.execute(f"select id, data from {table_name} WHERE id =
?", [key])
+ data_out[key] = cursor.fetchone()
+ if len(data_out) == 5:
+ time.sleep(7)
+
+ assert len(data_out) == row_count
+
+
+def test_heartbeat_disabled(table_name, drop_table_cleanup):
+ row_count = 10
+ with pyignite_dbapi.connect(address=server_addresses_basic[0],
heartbeat_interval=0) as conn:
+ with conn.cursor() as cursor:
+ cursor.execute(f"create table {table_name}(id int primary key,
data varchar)")
+ for key in range(row_count):
+ cursor.execute(f"insert into {table_name} values({key},
'data-{key*2}')")
+ assert cursor.rowcount == 1
+
+ data_out = {}
+ with pytest.raises(pyignite_dbapi.OperationalError) as err:
+ for key in range(row_count):
+ cursor.execute(f"select id, data from {table_name} where
id = ?", [key])
+ data_out[key] = cursor.fetchone()
+ if len(data_out) == 5:
+ time.sleep(7)
+
+ assert err.match("Connection closed by the server")
Review Comment:
This assertion is unreachable. The pytest.raises context manager on line 94
expects an exception to be raised within its scope. When the OperationalError
is raised inside the loop, execution will exit the context manager, and line
101 will never be executed. Consider removing this line or moving it outside
the pytest.raises block if you intended to verify the error message.
```suggestion
assert err.match("Connection closed by the server")
```
--
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]
