search

rails-sqlserver / tiny_tds

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

Raise error if client is unable to execute statement #469

Closed wpolicarpo closed 1 year ago

wpolicarpo commented 4 years ago

TinyTDS returns false when you try to execute a query and for some reason it fails (dead connection, a network intermittent error, etc). This PR changes this behavior to raise an exception instead.

This is particular useful for ORMs built on top of TinyTDS connection like Sequel or ActiveRecord. Note that this also breaks the public API and this might be undesired.

Fixes #451.

wpolicarpo commented 4 years ago

@aharpervc I'm not sure how to write a proper test case for this. I can see there is a test must run this test to prove we account for dropped connections being skipped now and looks like it should be testing something similar to what we need here. Any suggestions?

aharpervc commented 4 years ago

Yeah good question how can we force this behavior? What about something like this:

  1. create a connection
  2. run something like kill @@spid (not sure if this can be done all at once, might need to be queried, then templated, then executed)
  3. with that same connection, attempt to run select 1
  4. validate what happens when that fails
jeremyevans commented 4 years ago

Even if this can't be cleanly tested, it's clearly better to raise an exception instead of returning false. If you look at the README, it's clear that execute is never expected to return false, as none of the code that calls execute checks for false. And apparently, none of the specs check for it returning false either.

bf4 commented 3 years ago

Just came across this from the rails sql server adapter source. Is there a blocker to merging this? cc @metaskills

aharpervc commented 3 years ago

Just came across this from the rails sql server adapter source. Is there a blocker to merging this? cc @metaskills

There's no real hard blockers, however it would be great if we added a test for this. You can also contribute that test @bf4 if this is a priority for you. Or I will add a test at some point later this autumn when I have more time to look at it. Basically anyone can, or we can decide that no test is needed and merge regardless.

One question I currently have is the return self part of this. What does that mean for callers when this happens?

bf4 commented 3 years ago

Should be easy for me to test given my local docker server keeps dying mid request

On Wed, Sep 2, 2020, 12:43 PM Andrew Harper notifications@github.com wrote:

Just came across this from the rails sql server adapter source. Is there a blocker to merging this? cc @metaskills https://github.com/metaskills

There's no real hard blockers, however it would be great if we added a test for this. You can also contribute that test @bf4 https://github.com/bf4 if this is a priority for you.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/rails-sqlserver/tiny_tds/pull/469#issuecomment-685893847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABC4QVTVUVOXCFQMM2S5D3SDZ74FANCNFSM4NBQ356Q .

aharpervc commented 3 years ago

@wpolicarpo @bvogelzang was able to force various timeouts in https://github.com/rails-sqlserver/tiny_tds/pull/481, so maybe something can be done here that way? Alternately if there isn't enough energy to add a test, I would be okay merging regardless. I think the value is high enough on this one to do it anyway.

wpolicarpo commented 3 years ago

@aharpervc I'm struggling to find time to look into this and some other adapter issues these days, but I can look into adding some tests, yes. I was already following that PR to get some ideas and implement them the same way here.

If you are planning on cutting a release anytime soon, we could merge this the way it is and I can open a new PR adding tests after if you're OK with that.

wpolicarpo commented 3 years ago

@aharpervc I finally had time to look into this today. I think I will wait until #481 is merged and use the same approach to write some tests for this change. I could rebase my branch on top of that, but I would still need it to get merged first anyway.

Do you have a rough idea of when that PR will be merged?

aharpervc commented 3 years ago

Do you have a rough idea of when that PR will be merged?

I expect we can merge it when the CI tests pass. I don't recall off the top of my head what the blocker there was

aharpervc commented 3 years ago

I expect we can merge it when the CI tests pass. I don't recall off the top of my head what the blocker there was

@bvogelzang got the tests to pass, so we're moving forward. I included this PR in the 2.1.4-pre2 for validation: https://github.com/rails-sqlserver/tiny_tds/pull/486. I still think it'd be ideal to include a test in this PR if possible. Either way, can you add a bit to the "unreleased" section of the changelog @wpolicarpo?

wpolicarpo commented 3 years ago

Thanks @aharpervc and @bvogelzang. I'll look into adding a test using toxiproxy this week.

aharpervc commented 3 years ago

@wpolicarpo I saw you added a testing commit here https://github.com/rails-sqlserver/tiny_tds/pull/486/commits/1c2f56e5ff740c7a745bf566c455bacd6d1fbf1b, what do you think about adding it to this branch? Did Toxiproxy have the intended effect?

aharpervc commented 3 years ago

@wpolicarpo I moved your commit that added a test from https://github.com/rails-sqlserver/tiny_tds/pull/486 to here and rebased on latest master (I realize this may be slightly disruptive since I force pushed, but it can be fixed if it's a problem)

bvogelzang commented 3 years ago

@wpolicarpo I think changing the test so it uses the timeout toxic instead of the down toxic will get us what we want:

client = new_connection timeout: 2, port: 1234
assert_client_works(client)
action = lambda { client.execute("select 1").do }

# Use toxiproxy to close the TCP socket.
# We want TinyTds to fail to send the statement and raise an error immediately.
Toxiproxy[:sqlserver_test].toxic(:timeout, timeout: 0).apply do
  assert_raise_tinytds_error(action) do |e|
    assert_match %r{failed to execute statement}i, e.message, 'ignore if non-english test run'
  end
end
wuarmin commented 2 years ago

This PR would fix a bug in my web app, which would be very important for me. Can it be merged soon?

Thanks!

wuarmin commented 2 years ago

hey @bvogelzang is there a chance that this PR will be merged?

best regards

wpolicarpo commented 1 year ago

I'm closing this PR because I'm unable to find time to look into it better and get it to a state that we can merge. If someone is able/wants to make it happen, please feel free to use the idea/code in here and open a new PR.

jeremyevans commented 1 year ago

@wpolicarpo If you have time, can you clarify what about the PR made it unable to merge? I'm not sure about the test, but the client.c change looks very straight forward, and even without a test, as long as it doesn't break any CI tests, seems like an improvement that should be merged.

wpolicarpo commented 1 year ago

@jeremyevans as far as I can remember, there were a few tests failing (maybe not directly related to these changes but due to CI changes happening at the same time I opened it). I just can't find time to look into it and I don't want to hold anyone back if they wanted to make it happen and decided no to because there was already a PR for that.