Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor tiny_tds to avoid sharing DBPROCESS #571

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andyundso
Copy link
Member

Background

In order for tiny_tds to communicate with a MSSQL server using FreeTDS, they provide a DBPROCESS struct to do so in C land. The interaction with DBPROCESS require to follow an exact sequence:

  • Open a connection using dbopen.
  • Prepare a command buffer using dbcmd.
  • Send it to the server using dbsqlsend.
  • Acknowledge that the server acknowledge using dbsqlok.
  • Gather metadata using dbresults.
  • Fetch each row using dbnextrow or cancel the running results (dbcancel)
  • Close the client using dbclose if not cancelled.

Our insert and do method, currently implemented on the Result class, perform the entire sequence. However, with execute, this is intentionally not done to allow lazy-loading of results from the server. This can lead to errors, some intended, others not:

  1. You get an error message if you try to make another query without requesting all results first (intended error). Although it can have unintended side-effects. For example, if you call find on Result, it will abort the each early, therefore not all results are consumed and you cannot start a new query - you have to initialise a new Client.

  2. There is a scenario with threads where you force a crash, see the following Ruby code:

it 'raises error when query cannot be sent' do
  client = new_connection
  assert_client_works(client)
    
  thread1 = Thread.new do
    client.execute("WaitFor Delay '00:00:10'")
  end

  assert thread1.alive?    
  thread2 = Thread.new do
    assert_raises(TinyTds::Error) { client.execute("WaitFor Delay '00:00:02'") }
  end
    
  thread1.join
  thread2.join
end

This will result in the following crash:

ruby: mem.c:1202: tds_free_connection: Assertion `conn->in_net_tds == NULL' failed.
Aborted (core dumped)
rake aborted!
  1. Technically, DBPROCESS as well as some metadata of ours (like dbsqlsent) is part of the client instance in C land. If the garbage collector decided to sweep away the Client instance, the results can no longer be consumed. A reproduction of this is provided in Segementation Fault when reading from a closed connection #435.
results = TinyTds::Client.new(opts).execute('SELECT 42 as answer_to_life')
GC.start
puts results.to_a

This will yield a segmentation fault, since the Client instance is unreachable from the point of view of the garbage collector, and it gets deallocated.

  1. You can force a segmentation fault by closing the client before consuming the result. Closing the client deallocates the DBPROCESS as well as the metadata. See Segementation Fault when reading from a closed connection #435 for the reproduction script.

Proposed solution

The proposed solution in this PR removes the lazy-loading functionality of tiny_tds. insert, do and execute are moved to the Client class. The C code for the Result class is removed entirely, thus leading the Client class to have sole control over all C data structures. Result is now a PORO holding the results rows as well as couple of metadata, like fields.

Comment

I am leaving this in draft state for now. It would require the release of a new major, which we only just did. I also did not check all of the code yet - I am sure the implementation is not fully complete yet (e.g. return code is missing). I also need to test a couple of edge-cases with threads and garbage collector.

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