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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andyundso
Copy link
Contributor

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

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