Raise error if client is unable to execute statement #469

wpolicarpo · 2020-05-15T12:19:37Z

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 · 2020-05-15T12:22:33Z

@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?

TinyTDS returns false instead of raising an exception if connection fails while executing a query. Getting around this by raising an exception ourselves while this PR rails-sqlserver/tiny_tds#469 is not released. The solution came from #717 and #818, I am just adjusting the error message to make it the same as that TinyTDS PR.

aharpervc · 2020-05-15T13:15:21Z

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

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

jeremyevans · 2020-05-25T21:49:26Z

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 · 2020-09-02T15:59:13Z

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

aharpervc · 2020-09-02T17:43:15Z

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 · 2020-09-02T22:48:51Z

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 ***@***.***> 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 <#469 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABC4QVTVUVOXCFQMM2S5D3SDZ74FANCNFSM4NBQ356Q> .

aharpervc · 2021-02-22T17:45:15Z

@wpolicarpo @bvogelzang was able to force various timeouts in #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 · 2021-02-25T22:49:44Z

@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 · 2021-03-14T19:16:09Z

@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 · 2021-03-15T13:55:47Z

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 · 2021-03-22T20:02:54Z

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: #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 · 2021-03-23T11:28:01Z

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

aharpervc · 2021-04-12T19:57:44Z

@wpolicarpo I saw you added a testing commit here 1c2f56e, what do you think about adding it to this branch? Did Toxiproxy have the intended effect?

aharpervc · 2021-05-07T18:02:50Z

@wpolicarpo I moved your commit that added a test from #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 · 2021-05-13T19:41:24Z

@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 · 2021-09-20T18:48:35Z

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

Thanks!

wuarmin · 2021-12-06T09:47:17Z

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

best regards

wpolicarpo · 2023-03-23T13:16:13Z

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 · 2023-03-23T20:25:47Z

@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 · 2023-03-27T11:46:08Z

@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.

wpolicarpo requested a review from aharpervc May 15, 2020 12:19

wpolicarpo mentioned this pull request May 15, 2020

Adjust error message when connection is dead rails-sqlserver/activerecord-sqlserver-adapter#825

Merged

wpolicarpo mentioned this pull request May 15, 2020

Avoid exception when db-connection is lost rails-sqlserver/activerecord-sqlserver-adapter#717

Closed

wpolicarpo mentioned this pull request May 15, 2020

Undefined Method Error Caused By Active? rails-sqlserver/activerecord-sqlserver-adapter#707

Closed

aharpervc mentioned this pull request Mar 22, 2021

Prepare for 2.1.4 #486

Merged

10 tasks

wpolicarpo mentioned this pull request Apr 21, 2021

Rails 6.1: Raise ActiveRecord::ConnectionNotEstablished on calls to execute with a disconnected connection. rails-sqlserver/activerecord-sqlserver-adapter#903

Merged

wpolicarpo added 2 commits May 7, 2021 13:59

Raise error if client is unable to execute statement

0c47afe

Add test case for when connection is dead and tiny_tds raises an error

5b96c72

aharpervc force-pushed the raise-error-if-execute-fails branch from bb03ed7 to 5b96c72 Compare May 7, 2021 17:59

squash: update changelog

1dcf19c

wpolicarpo closed this Mar 23, 2023

This was referenced May 8, 2024

Result from a dead connection is false. Should it throw an exception? #464

Closed

Raise error if dbsqlsend fails #560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise error if client is unable to execute statement #469

Raise error if client is unable to execute statement #469

wpolicarpo commented May 15, 2020 •

edited

Loading

wpolicarpo commented May 15, 2020

aharpervc commented May 15, 2020

jeremyevans commented May 25, 2020

bf4 commented Sep 2, 2020

aharpervc commented Sep 2, 2020 •

edited

Loading

bf4 commented Sep 2, 2020 via email

aharpervc commented Feb 22, 2021

wpolicarpo commented Feb 25, 2021

wpolicarpo commented Mar 14, 2021

aharpervc commented Mar 15, 2021 •

edited

Loading

aharpervc commented Mar 22, 2021 •

edited

Loading

wpolicarpo commented Mar 23, 2021

aharpervc commented Apr 12, 2021

aharpervc commented May 7, 2021

bvogelzang commented May 13, 2021 •

edited

Loading

wuarmin commented Sep 20, 2021 •

edited

Loading

wuarmin commented Dec 6, 2021

wpolicarpo commented Mar 23, 2023

jeremyevans commented Mar 23, 2023

wpolicarpo commented Mar 27, 2023

Raise error if client is unable to execute statement #469

Raise error if client is unable to execute statement #469

Conversation

wpolicarpo commented May 15, 2020 • edited Loading

wpolicarpo commented May 15, 2020

aharpervc commented May 15, 2020

jeremyevans commented May 25, 2020

bf4 commented Sep 2, 2020

aharpervc commented Sep 2, 2020 • edited Loading

bf4 commented Sep 2, 2020 via email

aharpervc commented Feb 22, 2021

wpolicarpo commented Feb 25, 2021

wpolicarpo commented Mar 14, 2021

aharpervc commented Mar 15, 2021 • edited Loading

aharpervc commented Mar 22, 2021 • edited Loading

wpolicarpo commented Mar 23, 2021

aharpervc commented Apr 12, 2021

aharpervc commented May 7, 2021

bvogelzang commented May 13, 2021 • edited Loading

wuarmin commented Sep 20, 2021 • edited Loading

wuarmin commented Dec 6, 2021

wpolicarpo commented Mar 23, 2023

jeremyevans commented Mar 23, 2023

wpolicarpo commented Mar 27, 2023

wpolicarpo commented May 15, 2020 •

edited

Loading

aharpervc commented Sep 2, 2020 •

edited

Loading

aharpervc commented Mar 15, 2021 •

edited

Loading

aharpervc commented Mar 22, 2021 •

edited

Loading

bvogelzang commented May 13, 2021 •

edited

Loading

wuarmin commented Sep 20, 2021 •

edited

Loading