Skip to content

Conversation

@subrata-ms
Copy link
Contributor

@subrata-ms subrata-ms commented Nov 12, 2025

Work Item / Issue Reference

AB#40420


Summary

This pull request refactors the way temporary buffers are managed for setting connection attributes in the Connection class, improving memory safety and simplifying buffer handling. Instead of using static vectors to store temporary string and binary data, the code now uses member variables to hold these buffers directly within each Connection instance.

Buffer Management Refactor:

  • Removed static vectors (wstr_buffers, buffers) for storing temporary wide string and binary buffers, eliminating the need for manual buffer growth management and cleanup. [1] [2] [3]
  • Added member variables wstrStringBuffer and strBytesBuffer to the Connection class for managing temporary buffers per connection instance, improving encapsulation and memory safety.
  • Updated all relevant buffer usage in setAttribute to use these new member variables, replacing references to static buffers with instance variables. [1] [2] [3]

Code Simplification:

  • Removed unnecessary static buffer and related logic, such as buffer size limits and periodic cleanup, resulting in cleaner and more maintainable code. [1] [2] [3]

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR addresses a memory accumulation issue in the Connection::setAttribute method by replacing static vectors that indefinitely stored attribute values with class-level buffers. The static vectors were unnecessary since attribute values only need to persist for the duration of the ODBC API call.

Key changes:

  • Removed static std::vector<std::wstring> and std::vector<std::string> buffers that accumulated historical attribute values
  • Introduced two class-level member variables (wstrStringBuffer and strBytesBuffer) to temporarily hold attribute data
  • Removed buffer size management code (MAX_BUFFER_COUNT logic) that was previously needed to prevent unbounded growth

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
mssql_python/pybind/connection/connection.h Adds two class-level buffer member variables for storing string and byte attribute data
mssql_python/pybind/connection/connection.cpp Replaces static vector buffers with class-level buffers and removes buffer management logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@subrata-ms
Copy link
Contributor Author

@subrata-ms please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

@subrata-ms subrata-ms closed this Nov 12, 2025
@subrata-ms subrata-ms reopened this Nov 12, 2025
@subrata-ms
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Microsoft"

@subrata-ms subrata-ms changed the title Handling Access token with class level variables and removing static defination FIX:Handling Access token with class level variables and removing static defination Nov 12, 2025
@subrata-ms subrata-ms changed the title FIX:Handling Access token with class level variables and removing static defination FIX: Handling Access token with class level variables and removing static defination Nov 12, 2025
@github-actions github-actions bot added the pr-size: small Minimal code update label Nov 12, 2025
Copy link
Contributor

@sumitmsft sumitmsft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple of comments.

@github-actions
Copy link

github-actions bot commented Nov 12, 2025

📊 Code Coverage Report

🔥 Diff Coverage

50%


🎯 Overall Coverage

77%


📈 Total Lines Covered: 4819 out of 6194
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pybind/connection/connection.cpp (50.0%): Missing lines 258-261

Summary

  • Total: 8 lines
  • Missing: 4 lines
  • Coverage: 50%

mssql_python/pybind/connection/connection.cpp

Lines 254-265

  254     } else if (py::isinstance<py::bytes>(value) ||
  255                py::isinstance<py::bytearray>(value)) {
  256         try {
  257             std::string binary_data = value.cast<std::string>();
! 258             this->strBytesBuffer.clear();
! 259             this->strBytesBuffer = std::move(binary_data);
! 260             SQLPOINTER ptr = const_cast<char*>(this->strBytesBuffer.c_str());
! 261             SQLINTEGER length = static_cast<SQLINTEGER>(this->strBytesBuffer.size());
  262 
  263             SQLRETURN ret = SQLSetConnectAttr_ptr(_dbcHandle->get(),
  264                                                   attribute, ptr, length);
  265             if (!SQL_SUCCEEDED(ret)) {


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.helpers.py: 67.8%
mssql_python.pybind.ddbc_bindings.cpp: 70.7%
mssql_python.pybind.connection.connection.cpp: 75.9%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.ddbc_bindings.h: 79.7%
mssql_python.auth.py: 87.1%
mssql_python.pooling.py: 87.7%
mssql_python.__init__.py: 90.9%
mssql_python.exceptions.py: 92.8%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@subrata-ms subrata-ms removed the request for review from jahnvi480 November 12, 2025 18:51
@subrata-ms subrata-ms merged commit 36c9bed into main Nov 13, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: small Minimal code update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants