Skip to content

Commit 6af4610

Browse files
authored
FIX: Resource cleanup on Python shutdown to avoid segfaults (#255)
1 parent 9fcfe87 commit 6af4610

File tree

2 files changed

+280
-19
lines changed

2 files changed

+280
-19
lines changed

mssql_python/pybind/ddbc_bindings.cpp

Lines changed: 71 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -665,24 +665,67 @@ void HandleZeroColumnSizeAtFetch(SQLULEN& columnSize) {
665665

666666
} // namespace
667667

668+
// Helper function to check if Python is shutting down or finalizing
669+
// This centralizes the shutdown detection logic to avoid code duplication
670+
static bool is_python_finalizing() {
671+
try {
672+
if (Py_IsInitialized() == 0) {
673+
return true; // Python is already shut down
674+
}
675+
676+
py::gil_scoped_acquire gil;
677+
py::object sys_module = py::module_::import("sys");
678+
if (!sys_module.is_none()) {
679+
// Check if the attribute exists before accessing it (for Python version compatibility)
680+
if (py::hasattr(sys_module, "_is_finalizing")) {
681+
py::object finalizing_func = sys_module.attr("_is_finalizing");
682+
if (!finalizing_func.is_none() && finalizing_func().cast<bool>()) {
683+
return true; // Python is finalizing
684+
}
685+
}
686+
}
687+
return false;
688+
} catch (...) {
689+
std::cerr << "Error occurred while checking Python finalization state." << std::endl;
690+
// Be conservative - don't assume shutdown on any exception
691+
// Only return true if we're absolutely certain Python is shutting down
692+
return false;
693+
}
694+
}
695+
668696
// TODO: Revisit GIL considerations if we're using python's logger
669697
template <typename... Args>
670698
void LOG(const std::string& formatString, Args&&... args) {
671-
py::gil_scoped_acquire gil; // <---- this ensures safe Python API usage
699+
// Check if Python is shutting down to avoid crash during cleanup
700+
if (is_python_finalizing()) {
701+
return; // Python is shutting down or finalizing, don't log
702+
}
703+
704+
try {
705+
py::gil_scoped_acquire gil; // <---- this ensures safe Python API usage
672706

673-
py::object logger = py::module_::import("mssql_python.logging_config").attr("get_logger")();
674-
if (py::isinstance<py::none>(logger)) return;
707+
py::object logger = py::module_::import("mssql_python.logging_config").attr("get_logger")();
708+
if (py::isinstance<py::none>(logger)) return;
675709

676-
try {
677-
std::string ddbcFormatString = "[DDBC Bindings log] " + formatString;
678-
if constexpr (sizeof...(args) == 0) {
679-
logger.attr("debug")(py::str(ddbcFormatString));
680-
} else {
681-
py::str message = py::str(ddbcFormatString).format(std::forward<Args>(args)...);
682-
logger.attr("debug")(message);
710+
try {
711+
std::string ddbcFormatString = "[DDBC Bindings log] " + formatString;
712+
if constexpr (sizeof...(args) == 0) {
713+
logger.attr("debug")(py::str(ddbcFormatString));
714+
} else {
715+
py::str message = py::str(ddbcFormatString).format(std::forward<Args>(args)...);
716+
logger.attr("debug")(message);
717+
}
718+
} catch (const std::exception& e) {
719+
std::cerr << "Logging error: " << e.what() << std::endl;
683720
}
721+
} catch (const py::error_already_set& e) {
722+
// Python is shutting down or in an inconsistent state, silently ignore
723+
(void)e; // Suppress unused variable warning
724+
return;
684725
} catch (const std::exception& e) {
685-
std::cerr << "Logging error: " << e.what() << std::endl;
726+
// Any other error, ignore to prevent crash during cleanup
727+
(void)e; // Suppress unused variable warning
728+
return;
686729
}
687730
}
688731

@@ -993,17 +1036,26 @@ SQLSMALLINT SqlHandle::type() const {
9931036
*/
9941037
void SqlHandle::free() {
9951038
if (_handle && SQLFreeHandle_ptr) {
996-
const char* type_str = nullptr;
997-
switch (_type) {
998-
case SQL_HANDLE_ENV: type_str = "ENV"; break;
999-
case SQL_HANDLE_DBC: type_str = "DBC"; break;
1000-
case SQL_HANDLE_STMT: type_str = "STMT"; break;
1001-
case SQL_HANDLE_DESC: type_str = "DESC"; break;
1002-
default: type_str = "UNKNOWN"; break;
1039+
// Check if Python is shutting down using centralized helper function
1040+
bool pythonShuttingDown = is_python_finalizing();
1041+
1042+
// CRITICAL FIX: During Python shutdown, don't free STMT handles as their parent DBC may already be freed
1043+
// This prevents segfault when handles are freed in wrong order during interpreter shutdown
1044+
// Type 3 = SQL_HANDLE_STMT, Type 2 = SQL_HANDLE_DBC, Type 1 = SQL_HANDLE_ENV
1045+
if (pythonShuttingDown && _type == 3) {
1046+
_handle = nullptr; // Mark as freed to prevent double-free attempts
1047+
return;
10031048
}
1049+
1050+
// Always clean up ODBC resources, regardless of Python state
10041051
SQLFreeHandle_ptr(_type, _handle);
10051052
_handle = nullptr;
1006-
// Don't log during destruction - it can cause segfaults during Python shutdown
1053+
1054+
// Only log if Python is not shutting down (to avoid segfault)
1055+
if (!pythonShuttingDown) {
1056+
// Don't log during destruction - even in normal cases it can be problematic
1057+
// If logging is needed, use explicit close() methods instead
1058+
}
10071059
}
10081060
}
10091061

tests/test_005_connection_cursor_lifecycle.py

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,3 +475,212 @@ def test_mixed_cursor_cleanup_scenarios(conn_str, tmp_path):
475475
assert "All tests passed" in result.stdout
476476
# Should not have error logs
477477
assert "Exception during cursor cleanup" not in result.stderr
478+
479+
480+
def test_sql_syntax_error_no_segfault_on_shutdown(conn_str):
481+
"""Test that SQL syntax errors don't cause segfault during Python shutdown"""
482+
# This test reproduces the exact scenario that was causing segfaults
483+
escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')
484+
code = f"""
485+
from mssql_python import connect
486+
487+
# Create connection
488+
conn = connect("{escaped_conn_str}")
489+
cursor = conn.cursor()
490+
491+
# Execute invalid SQL that causes syntax error - this was causing segfault
492+
cursor.execute("syntax error")
493+
494+
# Don't explicitly close cursor/connection - let Python shutdown handle cleanup
495+
print("Script completed, shutting down...") # This would NOT print anyways
496+
# Segfault would happen here during Python shutdown
497+
"""
498+
499+
# Run in subprocess to catch segfaults
500+
result = subprocess.run(
501+
[sys.executable, "-c", code],
502+
capture_output=True,
503+
text=True
504+
)
505+
506+
# Should not segfault (exit code 139 on Unix, 134 on macOS)
507+
assert result.returncode == 1, f"Expected exit code 1 due to syntax error, but got {result.returncode}. STDERR: {result.stderr}"
508+
509+
def test_multiple_sql_syntax_errors_no_segfault(conn_str):
510+
"""Test multiple SQL syntax errors don't cause segfault during cleanup"""
511+
escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')
512+
code = f"""
513+
from mssql_python import connect
514+
515+
conn = connect("{escaped_conn_str}")
516+
517+
# Multiple cursors with syntax errors
518+
cursors = []
519+
for i in range(3):
520+
cursor = conn.cursor()
521+
cursors.append(cursor)
522+
cursor.execute(f"invalid sql syntax {{i}}")
523+
524+
# Mix of syntax errors and valid queries
525+
cursor_valid = conn.cursor()
526+
cursor_valid.execute("SELECT 1")
527+
cursor_valid.fetchall()
528+
cursors.append(cursor_valid)
529+
530+
# Don't close anything - test Python shutdown cleanup
531+
print("Multiple syntax errors handled, shutting down...")
532+
"""
533+
534+
result = subprocess.run(
535+
[sys.executable, "-c", code],
536+
capture_output=True,
537+
text=True
538+
)
539+
540+
assert result.returncode == 1, f"Expected exit code 1 due to syntax errors, but got {result.returncode}. STDERR: {result.stderr}"
541+
542+
543+
def test_connection_close_during_active_query_no_segfault(conn_str):
544+
"""Test closing connection while cursor has pending results doesn't cause segfault"""
545+
escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')
546+
code = f"""
547+
from mssql_python import connect
548+
549+
# Create connection and cursor
550+
conn = connect("{escaped_conn_str}")
551+
cursor = conn.cursor()
552+
553+
# Execute query but don't fetch results - leave them pending
554+
cursor.execute("SELECT COUNT(*) FROM sys.objects")
555+
556+
# Close connection while results are still pending
557+
# This tests handle cleanup when STMT has pending results but DBC is freed
558+
conn.close()
559+
560+
print("Connection closed with pending cursor results")
561+
# Cursor destructor will run during normal cleanup, not shutdown
562+
"""
563+
564+
result = subprocess.run(
565+
[sys.executable, "-c", code],
566+
capture_output=True,
567+
text=True
568+
)
569+
570+
# Should not segfault - should exit cleanly
571+
assert result.returncode == 0, f"Expected clean exit, but got exit code {result.returncode}. STDERR: {result.stderr}"
572+
assert "Connection closed with pending cursor results" in result.stdout
573+
574+
575+
def test_concurrent_cursor_operations_no_segfault(conn_str):
576+
"""Test concurrent cursor operations don't cause segfaults or race conditions"""
577+
escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')
578+
code = f"""
579+
import threading
580+
from mssql_python import connect
581+
582+
conn = connect("{escaped_conn_str}")
583+
results = []
584+
exceptions = []
585+
586+
def worker(thread_id):
587+
try:
588+
for i in range(15):
589+
cursor = conn.cursor()
590+
cursor.execute(f"SELECT {{thread_id * 100 + i}} as value")
591+
result = cursor.fetchone()
592+
results.append(result[0])
593+
# Don't explicitly close cursor - test concurrent destructors
594+
except Exception as e:
595+
exceptions.append(f"Thread {{thread_id}}: {{e}}")
596+
597+
# Create multiple threads doing concurrent cursor operations
598+
threads = []
599+
for i in range(4):
600+
t = threading.Thread(target=worker, args=(i,))
601+
threads.append(t)
602+
t.start()
603+
604+
for t in threads:
605+
t.join()
606+
607+
print(f"Completed: {{len(results)}} results, {{len(exceptions)}} exceptions")
608+
609+
# Report any exceptions for debugging
610+
for exc in exceptions:
611+
print(f"Exception: {{exc}}")
612+
613+
print("Concurrent operations completed")
614+
"""
615+
616+
result = subprocess.run(
617+
[sys.executable, "-c", code],
618+
capture_output=True,
619+
text=True
620+
)
621+
622+
# Should not segfault
623+
assert result.returncode == 0, f"Expected clean exit, but got exit code {result.returncode}. STDERR: {result.stderr}"
624+
assert "Concurrent operations completed" in result.stdout
625+
626+
# Check that most operations completed successfully
627+
# Allow for some exceptions due to threading, but shouldn't be many
628+
output_lines = result.stdout.split('\n')
629+
completed_line = [line for line in output_lines if 'Completed:' in line]
630+
if completed_line:
631+
# Extract numbers from "Completed: X results, Y exceptions"
632+
import re
633+
match = re.search(r'Completed: (\d+) results, (\d+) exceptions', completed_line[0])
634+
if match:
635+
results_count = int(match.group(1))
636+
exceptions_count = int(match.group(2))
637+
# Should have completed most operations (allow some threading issues)
638+
assert results_count >= 50, f"Too few successful operations: {results_count}"
639+
assert exceptions_count <= 10, f"Too many exceptions: {exceptions_count}"
640+
641+
642+
def test_aggressive_threading_abrupt_exit_no_segfault(conn_str):
643+
"""Test abrupt exit with active threads and pending queries doesn't cause segfault"""
644+
escaped_conn_str = conn_str.replace('\\', '\\\\').replace('"', '\\"')
645+
code = f"""
646+
import threading
647+
import sys
648+
import time
649+
from mssql_python import connect
650+
651+
conn = connect("{escaped_conn_str}")
652+
653+
def aggressive_worker(thread_id):
654+
'''Worker that creates cursors with pending results and doesn't clean up'''
655+
for i in range(8):
656+
cursor = conn.cursor()
657+
# Execute query but don't fetch - leave results pending
658+
cursor.execute(f"SELECT COUNT(*) FROM sys.objects WHERE object_id > {{thread_id * 1000 + i}}")
659+
660+
# Create another cursor immediately without cleaning up the first
661+
cursor2 = conn.cursor()
662+
cursor2.execute(f"SELECT TOP 3 * FROM sys.objects WHERE object_id > {{thread_id * 1000 + i}}")
663+
664+
# Don't fetch results, don't close cursors - maximum chaos
665+
time.sleep(0.005) # Let other threads interleave
666+
667+
# Start multiple daemon threads
668+
for i in range(3):
669+
t = threading.Thread(target=aggressive_worker, args=(i,), daemon=True)
670+
t.start()
671+
672+
# Let them run briefly then exit abruptly
673+
time.sleep(0.3)
674+
print("Exiting abruptly with active threads and pending queries")
675+
sys.exit(0) # Abrupt exit without joining threads
676+
"""
677+
678+
result = subprocess.run(
679+
[sys.executable, "-c", code],
680+
capture_output=True,
681+
text=True
682+
)
683+
684+
# Should not segfault - should exit cleanly even with abrupt exit
685+
assert result.returncode == 0, f"Expected clean exit, but got exit code {result.returncode}. STDERR: {result.stderr}"
686+
assert "Exiting abruptly with active threads and pending queries" in result.stdout

0 commit comments

Comments
 (0)