rails-sqlserver / tiny_tds

TinyTDS - Simple and fast FreeTDS bindings for Ruby using DB-Library.
Other
607 stars 189 forks source link

Unable to cancel after failed execute('exec sp_helptext xXxXx') #27

Closed raels closed 12 years ago

raels commented 13 years ago

In the active-record-adapter, there is the following code:

     select_values("EXEC sp_helptext #{quote_table_name(table_name)}").join

After chasing down a lot of code, it looks like there is a problem in tiny_tds..

Here is an example:

begin

r = c.execute('select x as foo')

    r = c.execute('exec sp_helptext xXxXx')
    r.each { |e| pp ["e", e] }

rescue r.cancel end

Executing the code above will fail with r being nil. Executing the code above it will succeed canceling r.

The problem is that once the sp_helptext has failed, nothing else on that connection can succeed.

Any help appreciated.

metaskills commented 13 years ago

Ah, I should be able to come up with fix for this soon.

metaskills commented 13 years ago

Looking into this today. In general the code above is OK. Here is some pseudo code to look over.

c = TinyTds::Client.new :host => '...', :username => '...', :password => '...'

r = c.execute("FOO BAR")  # => #<TinyTds::Result:0x00000101b61398>
r.each
TinyTds::Error: Could not find stored procedure 'FOO'.
    from (irb):5:in `each'
    from (irb):5
r.cancel # => true

r = c.execute("SELECT 1 AS [one]") # => #<TinyTds::Result:0x00000101b58568>
r.each # => [{"one"=>1}]

begin
  h = c.execute('exec sp_helptext xXxXx')
  h.each
rescue
  h.cancel
end

h # => #<TinyTds::Result:0x00000101b4cb00>
c.canceled? # => true

Basically the pattern is good and the handle will not be nil, at least in this contrived example.

I think you are proposing a SQL Server bug if it is not using TinyTDS correctly which could be possible in #execute_procedure since it does not use an ensure block for #finish_statement_handle

metaskills commented 13 years ago

I mean SQL Server Adapter bug, specifically with #execute_procedure and not always making sure we finish a statement handle.

raels commented 13 years ago

What I found was that the exec failed within the execute, which raised an exception, so the handle was never set, so cancel was sent to a nil pointer. I will go check the code...

in database_schema.rb, views definition information is collected with this:

    select_values("EXEC sp_helptext #{quote_table_name(table_name)}").join

That results in a call to raw_select, which further results in a call to raw_connection_do:

    def raw_connection_do(sql)
      case @connection_options[:mode]
      when :dblib
        @connection.execute(sql).do
      when :odbc
        @connection.do(sql)
      end
    ensure
      @update_sql = false
    end

This returns the result handle normally. However, since there is a "do", it fails inside execute, and there is no handle to return -- thus it becomes nil.

... at least that is what I think is going on.

Given that there appears to be no way to recover the TDS connection once in this state, I figured it was a TDS error as opposed to an AR adapter error. Seems like the result handle should always be available, or it should be possible to cancel through the connection as opposed to the result handle.

raels commented 13 years ago

Rats! Didn't mean to close it.... sorry!

metaskills commented 13 years ago

NP, lemme know what you find out and if needed open a ticket in the adapter. I can definitely see how I do not account for finishing the statement handle in the #execute_procedure method.

metaskills commented 13 years ago

So I may have touched on this issue over the weekend. I noticed that failures in sp_executesql for the 3.1 adapter would not allow the connection to be reused under FreeTDS 0.82. I wrote this test https://github.com/rails-sqlserver/tiny_tds/blob/master/test/result_test.rb#L600 this weekend and it fails only under 0.82. Under 0.91 it works just fine.

should 'error gracefully with incorrect syntax in sp_executesql' do
  if @client.freetds_091_or_higer?
    action = lambda { @client.execute("EXEC sp_executesql N'this will not work'").each }
    assert_raise_tinytds_error(action) do |e|
      assert_match %r|incorrect syntax|i, e.message
      assert_equal 15, e.severity
      assert_equal 156, e.db_error_number
    end
    assert_followup_query
  else
    skip 'FreeTDS 0.91 and higher can only pass this test.'
  end
end

I have also noticed that if I use 0.82 tsql command line utility, that it can cope, but tsql is a mixed bag of code that might not be under the same constraints of the DBLIB interface we are under.

1> EXEC sp_executesql N'this will not work'
2> GO
Msg 156, Level 15, State 1, Server SQLSERVER08, Line 1
Incorrect syntax near the keyword 'not'.
(return status = 156)
1> SELECT 1 AS [one]
2> GO
one
1

So, I'd like to play around with this a bit more to see if we can make TinyTDS cope a bit better. A few thoughts.

1) We do a lot of work to make sure a bad command batch calls both dbsqlok() and dbcancel() C functions. We do this in the exception handler because it is highly possible that no handle will be returned.

2) Can you confirm that your issue is related to this? Perhaps try writing a test that fails more like your example above and seeing if it does not fail under 0.91. FYI, the TinyTDS project allows you to compile local FreeTDS during it's rake test process. See the README, so no need to install a new system version to test 0.91RC2.

raels commented 13 years ago

This is similar to my issue, perhaps the same. The problem that I have is that the permissions for sp_helptext have been withheld as a security measure, so this sproc will always fail. The problem is that the failure causes no handle to be returned, so the dbproc can't be closed. (Sorry, I am responding to this from memory).

I will try an updated version as soon as I can.

raels commented 13 years ago

Meh... sorry. Issue still open...

metaskills commented 12 years ago

Gonna close this as a won't fix bug since it is 0.82 only.

raels commented 12 years ago

No worries. Thanks!