Skip to content

Stop using dbcancel as the nogvl unblock function - #607

Open
emailrhoads wants to merge 2 commits into
rails-sqlserver:masterfrom
emailrhoads:fix/null-nogvl-ubf
Open

emailrhoads wants to merge 2 commits into
rails-sqlserver:masterfrom
emailrhoads:fix/null-nogvl-ubf

Conversation

@emailrhoads

Copy link
Copy Markdown

Summary

  • rb_thread_call_without_gvl currently uses dbcancel as the unblock function. MRI invokes that UBF whenever the waiting thread has a pending interrupt, including process-directed signals such as SIGCHLD delivered to main after another thread spawns a child.
  • dbcancel then aborts the in-flight batch. Result#each treats FAIL as an empty success, so long queries can return [] even though SQL Server produced rows.
  • This keeps releasing the GVL (NULL, NULL) so other Ruby threads can run, but no longer cancels SQL on those interrupts. Client :timeout is unchanged (dbsetinterrupt).
  • Tradeoff: Timeout.timeout / Thread#kill wait until the batch finishes or :timeout fires.

Test plan

  • New test: another thread reaps child processes during WAITFOR DELAY; the following SELECT 42 must still return a row.
  • Existing timeout tests still pass (:timeout via dbsetinterrupt).
  • CI on this PR.

MRI calls the UBF on any pending interrupt (including SIGCHLD on main),
which aborted in-flight batches and made Result#each return []. Client
:timeout still uses dbsetinterrupt.

Co-authored-by: Cursor <cursoragent@cursor.com>
@emailrhoads
emailrhoads marked this pull request as draft August 19, 2026 15:02
@emailrhoads
emailrhoads marked this pull request as ready for review August 19, 2026 15:17
@emailrhoads

Copy link
Copy Markdown
Author

@andyundso any thoughts on this idea?

@andyundso

Copy link
Copy Markdown
Member

@emailrhoads I am trying to grasp the consequence of this change. If you have some time and maybe dev environment running, can you add the following test to thread_test.rb and see what happens?

client = new_connection
assert_client_works(client)

thread = Thread.new do
  client.do("waitfor delay '00:00:05'")
end

sleep 0.1

time = Benchmark.measure do
  thread.kill
end

assert time.real < 5

Otherwise I might have some time on the weekend to see myself. Nulling out the unblock function would mean the call just continue to run indefinitely, but maybe I am missing something.

Andy asked whether kill still aborts WAITFOR after removing dbcancel
as UBF. Kill returns immediately; join waits for the batch (~5s).

Co-authored-by: Cursor <cursoragent@cursor.com>
@emailrhoads

emailrhoads commented Sep 16, 2026

Copy link
Copy Markdown
Author

@emailrhoads I am trying to grasp the consequence of this change. If you have some time and maybe dev environment running, can you add the following test to thread_test.rb and see what happens?

client = new_connection
assert_client_works(client)

thread = Thread.new do
  client.do("waitfor delay '00:00:05'")
end

sleep 0.1

time = Benchmark.measure do
  thread.kill
end

assert time.real < 5

Otherwise I might have some time on the weekend to see myself. Nulling out the unblock function would mean the call just continue to run indefinitely, but maybe I am missing something.

@andyundso added the test and a bit more which I think was the spirit of your request at https://github.com/rails-sqlserver/tiny_tds/pull/607/changes#diff-fe76b289030e9af8010639512c4d9fab1f5cfd43207ae9137850ae1949dafae1R82

In short, we lose the ability to do Thread#kill and Timeout.timeout with this change and have it behave the way we would like.
Thread.kill will return quickly, but at Thread.join we are forced to wait for completion anyway.

Here are the actual test results though

Branch Thread#kill join after kill
master baseline (#610) ~0.0s ~0.01s (cancelled)
NULL UBF (#607) ~0.0s ~4.9s (WAITFOR ran)

The caller would need to use something like client.cancel to break the running connection instead.

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.

2 participants