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

Simplify SNIProxy #934

Merged
merged 3 commits into from
Jun 14, 2021
Merged

Simplify SNIProxy #934

merged 3 commits into from
Jun 14, 2021

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Feb 26, 2021

SniProxy is used as a single instance and called like this:

internal override uint SniGetConnectionId(ref Guid clientConnectionId) => SNIProxy.GetInstance().GetConnectionId(Handle, ref clientConnectionId);

This involves calling the GetInstance() method to get the single instance and then calling a function on that instance which only uses the parameters that are passed in. In these cases the functions could be static or merged into the callsite if there is only one use of the function.

The only thing that can't be removed from this single instance pattern is the GetLastError functions. I've investigated what it would take to remove it and it's very very complicated and involves significant changes to the StateObject interface between shared and platform implementations. I choose not to do that now.

Base automatically changed from master to main March 15, 2021 17:54
@Wraith2
Copy link
Contributor Author

Wraith2 commented May 28, 2021

@cheenamalhotra @David-Engel this has been open 3 months now. It's trivial and passed all tests when it was opened.

@cheenamalhotra
Copy link
Member

Hi @Wraith2

We'll be adding this and many more PRs to the next release cycle preview milestone after planning happens, as at this point driver code is under security testing and preparation for GA 3.0 and only already planned changes are considered right now.

In a week or two, you can expect activities resuming on PRs.

@Wraith2 Wraith2 mentioned this pull request Jun 11, 2021
@JRahnama JRahnama added this to the 4.0.0-preview1 milestone Jun 11, 2021
@JRahnama
Copy link
Contributor

@Wraith2 can you address the conflict please?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jun 11, 2021

Done, if it hadn't been left so long there wouldn't have been conflicts to deal with.

Copy link
Contributor

@DavoudEshtehari DavoudEshtehari left a comment

Choose a reason for hiding this comment

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

I couldn't find any usage for SspiClientContextResult, I believe it can be removed.
CC @cheenamalhotra

@cheenamalhotra
Copy link
Member

I couldn't find any usage for SspiClientContextResult, I believe it can be removed.
CC @cheenamalhotra

I think this would have been added before NegotiateStreamPal was added and it stayed as is.. I don't see any use of this class either.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jun 14, 2021

SspiClientContextResult removed.

@cheenamalhotra
Copy link
Member

Some build failures:
E:\ci-agent1\_work\4\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft\Data\SqlClient\TdsParser.Unix.cs(29,42): error CS0117: 'SNIProxy' does not contain a definition for 'GetInstance' [E:\ci-agent1\_work\4\s\src\Microsoft.Data.SqlClient\netcore\src\Microsoft.Data.SqlClient.csproj]

@Wraith2
Copy link
Contributor Author

Wraith2 commented Jun 14, 2021

Fixed and CI is green.

@cheenamalhotra cheenamalhotra merged commit 218e34f into dotnet:main Jun 14, 2021
@Wraith2
Copy link
Contributor Author

Wraith2 commented Jun 14, 2021

yay!

@Wraith2 Wraith2 deleted the rework-sniproxy branch June 29, 2021 21:54
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.

4 participants