Refactor tiny_tds to avoid sharing DBPROCESS
#571
Closed
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_tdsto communicate with a MSSQL server usingFreeTDS, they provide aDBPROCESSstruct to do so in C land. The interaction withDBPROCESSrequire to follow an exact sequence:dbopen.dbcmd.dbsqlsend.dbsqlok.dbresults.dbnextrowor cancel the running results (dbcancel)dbcloseif not cancelled.Our
insertanddomethod, currently implemented on theResultclass, 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
findonResult, it will abort theeachearly, 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:
DBPROCESSas well as some metadata of ours (likedbsqlsent) is part of the client instance in C land. If the garbage collector decided to sweep away theClientinstance, 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
Clientinstance is unreachable from the point of view of the garbage collector, and it gets deallocated.DBPROCESSas 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,doandexecuteare moved to theClientclass. The C code for theResultclass is removed entirely, thus leading theClientclass to have sole control over all C data structures.Resultis 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.