Fix infinite loop when end of stream is reached

[alexdougie]

This is to fix issue #165

We're seeing this issue in our production and development environments. We're using .NET Core 3.1 on CentOS 7. A reliable way to reproduce it is calling SqlConnection.Open() every half a second while Windows Server is restarting. This causes the process to go up to 100% CPU usage, which as you can imagine can cause problems. 😄

We've tested against our environment and this fixes the problem, as the exception is caught further up and another is thrown out of SqlConnection.Open().

Of course I am open to different approaches if this isn't the preferred way to handle it.

• I'm not entirely happy with the variable name readBytesForHeader, any suggestions are welcome.
• The exception message seems a bit redundant given the name of the exception. Any preference to having the message or not?
• This is also a problem in System.Data.SqlClient, so it needs backporting. I'm not sure how this is done. Of course I'm happy to submit a PR to fix it there too if that's preferred.

[Wraith2]

Nice find. I was aware of this as a theoretical problem and have a fix in #541 but you've actually verified that it can happen in production on 3.1 which I didn't think was possible.

[cheenamalhotra]

Thanks for catching and confirming the fix @alexdougie !

We've not been able to reproduce the issue yet (also efforts ongoing in #567), without which we weren't sure if proposed fix (#165 (comment)) was accurate. But we can take your confirmation and also the fix isn't risky to implement so we can consider safely backporting it to v1.1.

We generally backport critical issues only to System.Data.SqlClient, and since we know the issue has greater impact, I think we can take it forward for backporting.

[karinazhou]

Hi @alexdougie, I am trying to reproduce the hanging behavior on my side but no luck for now. From your comments, is the trick to restart the windows server where the SQL Server lives on when executing the SqlConnection.Open() ?

[Wraith2]

The repro is tricky. You have to end up with a freshly opened (because encapsulation only applies on the first packet) managed sni connection which doesn't throw an exception when you call Read or ReadAsync on it which isn't supposed to be possible in 3.1 anymore see dotnet/runtime#36293 (comment) for related discussion. This combination is hard to engineer for obvious reasons which is why I thought it was theoretical until recently.

[alexdougie]

It is very tricky to reproduce. It took us a long time to narrow it down. I've stuck the repro code below that we're running while restarting Windows Server (just restarting the SQL server service doesn't seem to cause the issue). Even then, it actually only happens about 50% of the time so it can take a few tries to reproduce.

If #541 fixes it then I don't mind this PR being closed. So long as a fix is in the next version of SqlClient we'll be happy! 😄

class Program
    {
        static async Task Main(string[] args)
        {
            while (true)
            {
                try
                {
                    var connectionString = args[0];

                    await using var connection = new SqlConnection(connectionString);
                    connection.Open();
                    Console.WriteLine("Opened connection.");
                }
                catch (Exception e)
                {
                    Console.WriteLine(e);
                }
                finally
                {
                    await Task.Delay(500);
                }
            }
        }
    }

[karinazhou]

I tried this fix with Issue#567 which also triggers the hanging behavior. From the test, I can see the application goes into an infinite loop when readBytes remains unchanged inside the while loop in ReadInternal task.

The hanging can be reproduced both for .NET Core 2.1 and 3.1. One thing I am not sure about is what causes this End of Stream scenario.

[Wraith2]

So even though I'd already got an open PR to fix this you're going to merge this one? I really hope you're not going to complain that mine is out of date and request I update it to master because that would just be taking the piss at this point.

[cheenamalhotra]

@Wraith2

We will continue to review your PR in the next preview release cycle as it does bring lot of enhancements but also requires a thorough review and we would like to bring it in a preview version for end users to try on. Also we are currently wrapping up for GA 2.0 release, and are only picking up PRs that offer minimal changes and lowest risk for fixing important driver issues.