Refactor tiny_tds
to avoid sharing DBPROCESS
#571
Draft
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
In order for
tiny_tds
to communicate with a MSSQL server usingFreeTDS
, they provide aDBPROCESS
struct to do so in C land. The interaction withDBPROCESS
require to follow an exact sequence:dbopen
.dbcmd
.dbsqlsend
.dbsqlok
.dbresults
.dbnextrow
or cancel the running results (dbcancel
)dbclose
if not cancelled.Our
insert
anddo
method, currently implemented on theResult
class, perform the entire sequence. However, withexecute
, this is intentionally not done to allow lazy-loading of results from the server. This can lead to errors, some intended, others not: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
onResult
, it will abort theeach
early, therefore not all results are consumed and you cannot start a new query - you have to initialise a newClient
.There is a scenario with threads where you force a crash, see the following Ruby code:
This will result in the following crash:
DBPROCESS
as well as some metadata of ours (likedbsqlsent
) is part of the client instance in C land. If the garbage collector decided to sweep away theClient
instance, the results can no longer be consumed. A reproduction of this is provided in Segementation Fault when reading from a closed connection #435.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.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
andexecute
are moved to theClient
class. The C code for theResult
class is removed entirely, thus leading theClient
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, likefields
.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.