Refactor tiny_tds to avoid sharing DBPROCESS#571
Closed
andyundso wants to merge 5 commits intorails-sqlserver:masterfrom
Closed
Refactor tiny_tds to avoid sharing DBPROCESS#571andyundso wants to merge 5 commits intorails-sqlserver:masterfrom
tiny_tds to avoid sharing DBPROCESS#571andyundso wants to merge 5 commits intorails-sqlserver:masterfrom
Conversation
Open
Member
Author
|
Replaced with #595. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.