Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error if dbsqlsend fails #560

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

andyundso
Copy link
Member

@andyundso andyundso commented May 13, 2024

Closes #464
Relates to #552

With this PR, tiny_tds will raise an error in case dbsqlsend will return FAIL instead of returning false. It is essentially the code from #469 / @wpolicarpo.

I spent an evening trying to build a test that would yield this error. I came up with a solution that worked on my machine, but only sometimes on CI.

it 'raises error when query cannot be sent' do
  client = new_connection
  assert_client_works(client)

  thread1 = Thread.new do
    client.execute("SELECT 1 as [one]").do
  end

  thread2 = Thread.new do
    assert_equal false, client.execute("SELECT 1 as [one]")
  end

  thread1.join
  thread2.join
end

I assume, when the result are read in thread1 and Ruby decides to switch to thread2, our FreeTDS wrapper is in an invalid state, which can trigger the issue. Likely, because they both try to access the DBPROCESS object, which should not be allowed.

I also tried various suggestions to kill the session or just restart the database server, none of them worked for me to trigger the error. I personally think we can still get this code in even if it has no tests, as it can avoid getting into weird state with tiny_tds.

@andyundso andyundso requested a review from aharpervc May 13, 2024 20:41
@aharpervc
Copy link
Contributor

Idea, try adding sleep 1 in thread1 after the statement, eg

thread1 = Thread.new do
  client.execute("SELECT 1 as [one]").do
  sleep 1
end

@andyundso
Copy link
Member Author

I pushed the test now @aharpervc. As you can see, it sometimes works, sometimes not. So it really depends on a race-conditon between the two threads.

@aharpervc
Copy link
Contributor

Yeah, unfortunate, I thought we could force an order of operations with the sleep. It seemed to be reliable for me locally with that technique. Is there a way to kill the client without threads?

@andyundso
Copy link
Member Author

Hi @aharpervc

I experimented this evening again and I did not find any way to write a test that could simulate a FAIL from dbsqlsend. The primary case where this could happen is when the SQL server goes down between us checking if it is available with REQUIRE_OPEN_CLIENT and then running dbsqlsend, as illustrated below.

REQUIRE_OPEN_CLIENT(cwrap);
dbcmd(cwrap->client, StringValueCStr(sql));
if (dbsqlsend(cwrap->client) == FAIL) {
  rb_raise(cTinyTdsError, "failed dbsqlsend() function");
}

I assume the reason why the thread-based test sometimes work is because the userdata or the command buffer of FreeTDS is manipulated at the correct moment, and so the test fails. which, like we saw, is not reproducable really.

I now removed the commit with the test and suggest to merge this change as it is.

@andyundso andyundso merged commit 2673569 into rails-sqlserver:master Jul 13, 2024
88 of 89 checks passed
@andyundso andyundso deleted the raise-error branch July 13, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Result from a dead connection is false. Should it throw an exception?
2 participants