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

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

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.

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

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?

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

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

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

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

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?

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

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

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

@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

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

Thanks!

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

best regards

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.

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

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