rails-sqlserver / tiny_tds

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

multiple raised errors ... only last one captured #78

Open ghost opened 12 years ago

ghost commented 12 years ago

I have an app that uses extensive stored procs and with a few tweaks, tiny_tds works great (thanks!). I do have one issue that if a proc has multiple raised errors (happens a lot when procs call procs), tiny_tds only captures the last error. I believe it's due to the way tinytds_msg_handler calls rb_tinytds_raise_error which creates a new error instead of buffering them up.

I would love to submit a patch but I'm hitting the wall on my ruby internals knowledge (pointers welcome, I'm more than happy to tackle this if someone can point me in a halfway decent direction).

I do have this simple test case that displays the problem:

require 'test_helper'

class MultiErrorsTest < ActiveSupport::TestCase

  test 'should have multi errors' do

    @client = TinyTds::Client.new( 
      :dataserver => 'DEV', 
      :username => 'sa',
      :password => 'xxxxx',
      :tds_version => '42',
      :database => 'tempdb' 
    )

   begin
     @client.execute( 'DROP PROCEDURE inner_proc_test' ).do
     @client.execute( 'DROP PROCEDURE outer_proc_test' ).do
   rescue
     # do nothing ... we don't care about drop warnings
   end

   inner_proc_test  = @client.execute( "create procedure inner_proc_test as raiserror 20014 'Inner Proc Error'" ).do
   outer_proc_test  = @client.execute( "create procedure outer_proc_test as exec inner_proc_test raiserror 20014 'Outer Proc Error'" ).do

   begin 
     results = @client.execute( 'exec outer_proc_test' ).do
   rescue Exception => e
     assert_match( /inner/, e.message )
   end

  end
end

This will fail because e is just the error from the outer_proc_test raise .. inner_proc_test raise is lost (and leaked?)

metaskills commented 12 years ago

Wow, this is a damn good question. Can you investigate the TDS/DBLIB spec on this? I would start on the FreeTDS mailing list and go from there. Search for how the dberrhandle and/or dbmsghandle should handle this situation. Then we can correlate that with how ruby manages exceptions. Starting here may help too http://www.freetds.org/userguide/samplecode.htm

ghost commented 12 years ago

Looking at the freetds api and how DBD::Sybase does it, I'm pretty sure all the work is on the ruby side. Since the error and message handlers are set

  dberrhandle(tinytds_err_handler);
  dbmsghandle(tinytds_msg_handler);

the rb_tinytds_raise_error function can be called multiple times. What needs to happen on the ruby side (and where my Ruby-C foo fails), is we need to create the TinyTds::Error object on the first time and then on future calls append to that object rather than creating a new one.

I've given it a shot but I keep running into problems setting the message (since that appears to be set in StandardError and not TinyTds::Error -- I'm guesing here).

I'm still working it but any Ruby-C pointers you can throw my way would be appreciated.

Thans.

metaskills commented 12 years ago

Sadly, I lost all my pointers, I just had an 800 page DBLIB PDF document, a few ruby.h files I plucked from the source and tons of failed googling. Here is the PDF.

http://cdn.actionmoniker.com/share/DBLibrary.pdf

About the changes to TinyTDS. IIRC, creating an exception object pops the stack. Meaning any error down the line would be lost. Perhaps the changes would have to be done in a way that pushes errors to a collection then at the end of the read from server, would raise them if present. This would seem to get hairy to me as it would mean you could not rely on the message handler stopping and raising during say a server write. Looking forward to seeing what you come up with. Thanks!

ghost commented 12 years ago

Looks like I would have to

  1. add a field to tinytds_client_userdata ( VALUE messages )
  2. in tinytds_msg_handler unpack userdata
  3. add msgtext to userdata->messages (rb_ary_xxx)
  4. in rb_tinytds_raise_error, check userdata->messages and if any add it and error
  5. rb_exc_raise with new error string

if you think that has some value, I could code it up and submit a patch (or pull request)

metaskills commented 12 years ago

Sure, especially if it strengthens the library in regards to stored procedures and still passing the tests.

metaskills commented 10 years ago

BTW, the work done in issue #133 is now in master. It relates to how/if we solve this one.

aharpervc commented 3 years ago

I believe this will be addressed by https://github.com/rails-sqlserver/tiny_tds/pull/475