Skip to content

Conversation

shueybubbles
Copy link
Collaborator

Fixes #187
Now that go-mssqldb supports np and lpc protocols, so can go-sqlcmd, at least on Windows.

Copy link
Contributor

@apoorvdeshmukh apoorvdeshmukh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

"program": "${workspaceFolder}/cmd/sqlcmd",
"args" : ["-Q", "EXIT(select 100 as Count)"],
"program": "${workspaceFolder}/cmd/modern",
"args" : ["-Q", "EXIT(select network_protocol from sys.dm_exec_sessions)", "-S", "lpc:."],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we already have a unit test to test this scenario?

Copy link
Collaborator

@stuartpa stuartpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm 8 getting unit test failures on my local machine with this branch

=== RUN TestExitOnError
unable to get instances from Sql Server Browser on host localhost: read udp [::1]:63086->[::1]:1434: i/o timeout
sqlcmd_test.go:286:
Error Trace: C:\src\go-sqlcmd\cmd\sqlcmd\sqlcmd_test.go:286

I run with Browser disabled. If I enable browser, then the tests don't fail, but this PR should not require browser service be running moving forward.

We need a unit test to catch this in the future (make sure we don't approve a change that causes browser service running to be a dependency)

@apoorvdeshmukh
Copy link
Contributor

I'm 8 getting unit test failures on my local machine with this branch

=== RUN TestExitOnError unable to get instances from Sql Server Browser on host localhost: read udp [::1]:63086->[::1]:1434: i/o timeout sqlcmd_test.go:286: Error Trace: C:\src\go-sqlcmd\cmd\sqlcmd\sqlcmd_test.go:286

I run with Browser disabled. If I enable browser, then the tests don't fail, but this PR should not require browser service be running moving forward.

We need a unit test to catch this in the future (make sure we don't approve a change that causes browser service running to be a dependency)

Hi @stuartpa , how did you get these test failures? I just ran the tests locally and I all the unit tests passed. Do we need to do something to disable browser service?

@stuartpa
Copy link
Collaborator

net stop "SQL Server Browser"

to stop it

@shueybubbles
Copy link
Collaborator Author

the browser dependency will be fixed by tagging go-mssqldb to v0.20.0 and using that build, which has the fix.

@shueybubbles
Copy link
Collaborator Author

I'm going to sit on this PR for a while and work on some higher priority items (like NOTICE generation) and come back to it.
I'm encountering some problems with connections going idle over named pipes and not being handled correctly, and I need to figure out if it's something go-mssqldb can handle or if sqlcmd isn't handling the error correctly.

Copy link
Collaborator

@stuartpa stuartpa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@shueybubbles
Copy link
Collaborator Author

@stuartpa can you confirm I've fixed this on your system? Then I'll merge


In reply to: 1245441859

@shueybubbles shueybubbles merged commit e3e6e71 into main Feb 7, 2023
@shueybubbles shueybubbles deleted the shueybubbles/np branch February 7, 2023 22:10
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.

Update go-mssqldb and support named pipes and shared memory
3 participants