Skip to content

Conversation

@andyundso
Copy link
Member

@andyundso andyundso commented Sep 9, 2025

This PR refactors a few aspects around #new and #connect on the TinyTds::Client.

First, use keyword arguments instead of an opts hash. the list of arguments is quite long, but it allows users to take advantage of pattern matching when passing arguments to initialising a new client. The values are also preserved as instance variables in the TinyTds::Client, allowing to implement the second point of this PR.

Second, I removed calling #connect from #new. Instead, #connect is called from either #do, #insert or #execute shortly before the SQL is sent to the server (using active?, means a reconnect also happens automatically if the FreeTDS DBPROCESS died). I originally wanted that #connect has to be called manually, but from a usage-perspective, this is rather cumbersome, especially when we know best when #connect has to be called.

@andyundso andyundso requested a review from Copilot September 9, 2025 14:16
Copy link

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 refactors the TinyTds::Client initialization and connection handling by separating the #new and #connect methods and modernizing the API to use keyword arguments.

  • Switched from hash-based options to keyword arguments for TinyTds::Client.new
  • Separated connection logic from initialization - #connect is now called automatically before SQL execution
  • Implemented GVL (Global Virtual Machine Lock) handoff during connection operations for better multi-threading performance

Reviewed Changes

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

Show a summary per file
File Description
lib/tiny_tds/client.rb Refactored constructor to use keyword arguments and separated connection logic
ext/tiny_tds/client.c Updated C extension to support automatic connection before SQL execution and GVL handoff
test/test_helper.rb Updated test helper to use new keyword argument syntax
test/client_test.rb Updated tests to reflect new connection behavior and API changes
test/result_test.rb Minor test adjustment for new connection state handling
README.md Updated documentation to reflect keyword arguments and renamed appname to app_name
CHANGELOG.md Added changelog entries for the breaking changes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 1022 to 986
if (azure == Qtrue || contained == Qtrue) {
#ifdef DBSETLDBNAME
DBSETLDBNAME(cwrap->login, StringValueCStr(database));
Copy link

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

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

The removal of the #ifdef DBSETLDBNAME guard makes this code dependent on a specific FreeTDS version without fallback handling. Consider adding version checks or error handling for systems where DBSETLDBNAME is not available.

Copilot uses AI. Check for mistakes.
CHANGELOG.md Outdated
* `TinyTds::Client.new` now accepts keyword arguments instead of a hash
* Renamed `tds_version` and `tds_version_info` to `server_version` and `server_version_info`
* Separate `#new` and `#connect`
* Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed.+
Copy link

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

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

There's a trailing '+' character at the end of the line that should be removed.

Suggested change
* Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed.+
* Instead, before running `#do`, `#execute` or `#insert`, `tiny_tds` will check if the connection is active and re-connect if needed.

Copilot uses AI. Check for mistakes.
@andyundso andyundso force-pushed the separate-new-and-connect branch from cc06286 to 5a10d48 Compare September 9, 2025 14:19
@andyundso
Copy link
Member Author

Looks like this introduces an issue with the connection_pool / threading on Windows:

D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool/timed_stack.rb:65: [BUG] win32_mutex_lock: WAIT_ABANDONED
ruby 3.3.9 (2025-07-24 revision f5c772fc7c) [x64-mingw-ucrt]

-- Control frame information -----------------------------------------------
c:0008 p:---- s:0041 e:000040 CFUNC  :synchronize
c:0007 p:0049 s:0037 e:000036 METHOD D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool/timed_stack.rb:65
c:0006 p:0081 s:0030 e:000026 METHOD D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:160
c:0005 p:0006 s:0022 e:000021 BLOCK  D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:108 [FINISH]
c:0004 p:---- s:0018 e:000017 CFUNC  :handle_interrupt
c:0003 p:0015 s:0013 e:000012 METHOD D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:107
c:0002 p:0012 s:0008 e:000007 BLOCK  D:/a/tiny_tds/tiny_tds/test/thread_test.rb:28 [FINISH]
c:0001 p:---- s:0003 e:000002 DUMMY  [FINISH]

-- Ruby level backtrace information ----------------------------------------
D:/a/tiny_tds/tiny_tds/test/thread_test.rb:28:in `block (5 levels) in <class:ThreadTest>'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:107:in `with'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:107:in `handle_interrupt'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:108:in `block in with'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool.rb:160:in `checkout'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool/timed_stack.rb:65:in `pop'
D:/a/tiny_tds/tiny_tds/vendor/bundle/ruby/3.3.0/gems/connection_pool-2.5.4/lib/connection_pool/timed_stack.rb:65:in `synchronize'

-- Threading information ---------------------------------------------------
Total ractor count: 1
Ruby thread count for this ractor: 15

will need to setup a development environment on Windows to see what is going on.

@andyundso andyundso force-pushed the separate-new-and-connect branch from 57b9804 to 6e662e7 Compare September 9, 2025 18:37
@andyundso
Copy link
Member Author

I think I see it. I'll also have to do nogvl_setup and nogvl_cleanup for these two new functions. they need a DBPROCESS in order to find the userdata where we setup everything to not touch the Ruby VM during handing off the GVL. But for dbopen we do not have a DBPROCESS object yet, so some more refactoring needed. might need to rewrite this branch to first refactor this before trying again with the no GVL stuff.

(both tiny tds client wrapper and the DBPROCESS with dbgetuserdata point to the same userdata where these things are managed).

`encoding` denotes the actual `Encoding` then used to associate strings.
@andyundso andyundso force-pushed the separate-new-and-connect branch from 6e662e7 to 321c66a Compare September 10, 2025 18:22
@andyundso
Copy link
Member Author

I've removed handing off the GVL for now from this branch and will look into it with separate PRs to implement it step-by-step (first dbuse, then dbopen).

@andyundso andyundso merged commit ec25cc5 into rails-sqlserver:v4 Sep 10, 2025
107 of 108 checks passed
@andyundso andyundso deleted the separate-new-and-connect branch September 10, 2025 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant