search

alexbrainman / odbc

odbc driver written in go
BSD 3-Clause "New" or "Revised" License
352 stars 140 forks source link

Mark a connection bad if a transaction fails to start. #61

Closed ChrisHines closed 8 years ago

ChrisHines commented 8 years ago

I have recently been testing how this driver behaves during automatic fail-over when using SQL Server Availability Groups.

When testing from a Linux client using unixODBC 2.3.1 and FreeTDS 0.91 I was able to consistently reproduce an incorrect behavior by causing an unplanned database fail-over while a test client was attempting to update data in a transaction.

This driver correctly returned an *odbc.Error from Conn.Begin but does not set Conn.bad = true. The problem is that database/sql (Go 1.4.2) returns the connection to the pool and subsequent calls to Conn.Begin return errors.New("already in a transaction") and database/sql again returns the connection to the pool.

Applying this PR fixes the problem, but I cannot figure out how to reproduce the error in an automated test. I tried to test the scenario with the code in https://github.com/ChrisHines/odbc/commit/e833690e0e75d087122977811c2c0bbbdc499149 but it doesn't reproduce the problem. I think the behavior is different because the availability group listener keeps the network connection open during the fail-over and the ODBC client sees different errors in that case than the technique used by the test to break a proxied TCP connection.

@alexbrainman can you think of a way to test this? How can we make this line return an error?

    err := c.setAutoCommitAttr(api.SQL_AUTOCOMMIT_OFF)
alexbrainman commented 8 years ago

I don't have "SQL Server Availability Groups" so it is difficult for me to help you here. But your change looks reasonable. I agree new test would be nice.

How can we make this line return an error?

I take it you want artifically break c.setAutoCommitAttr during test. If so, then grep for testingIssue5. Might give you some ideas.

Alex

ChrisHines commented 8 years ago

I generally like to avoid the global variable hook for tests, but if that's required for this case I can make that work. I will update this PR with a test when I have it ready.

ChrisHines commented 8 years ago

@alexbrainman: I have updated this PR with a working test. The test is in a separate commit to make it easier to verify the test is correct before checking that the fix resolves the issue.

ChrisHines commented 8 years ago

Yeah, that was pretty bad. Hopefully it is better now.