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

KeepAlive not working #849

Closed
kartas39 opened this issue Dec 14, 2020 · 2 comments · Fixed by #854
Closed

KeepAlive not working #849

kartas39 opened this issue Dec 14, 2020 · 2 comments · Fixed by #854

Comments

@kartas39
Copy link

kartas39 commented Dec 14, 2020

internal static void SetKeepAliveValues(ref Socket socket)

This method is not called from anywhere. So I think it just doesn't do his stuff.
Please also pay attention that it is wrapped with if NETCOREAPP31_AND_ABOVE, which is defined as:

<PropertyGroup Condition="'$(TargetGroup)' == 'netcoreapp' AND !$(TargetFramework.StartsWith('netcoreapp2.'))">
    <DefineConstants>$(DefineConstants);NETCOREAPP31_AND_ABOVE</DefineConstants>
  </PropertyGroup>

which does not include .Net5 I assume if you build with net5.0

@karinazhou
Copy link
Member

Hi @kartas39 ,

I think you are right. This SetKeepAliveValues is not referenced anywhere in the driver. I checked the history of the code and it was initially called in System.Data.SqlClient :
https://github.com/dotnet/corefx/blob/release/3.1/src/System.Data.SqlClient/src/System/Data/SqlClient/SNI/SNITcpHandle.cs#L227

However, when we move from System.Data.SqlClient to Microsoft.Data.SqlClient on Github, this call has been removed. This SetKeepAliveValues method is added back in the driver in PR#395 which will support the TCP KeepAlive feature in the future.

Currently, the driver is not officially built against .NET5.0 but it is compatible when you use it in a .NET5.0 application. For a .net core application, the actual driver's runtime library is from \netcoreapp3.1 folder. The SNITcpHandle is part of the managed SNI implementation which is not used on Windows by default.

Thanks,

@cheenamalhotra
Copy link
Member

@kartas39

That was a miss, thanks for reporting it.
The fix has been merged and will be released in future versions.

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 a pull request may close this issue.

3 participants