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

SqlDataReader.ReadAsync and NextResultAsync cause unobserved task exception even when awaited #421

Closed
jnm2 opened this Issue Jun 13, 2017 · 36 comments

Comments

Projects
None yet
2 participants
@jnm2
Contributor

jnm2 commented Jun 13, 2017

It's now been fourteen months, two new versions of .NET (4.6.2, 4.7) and many monthly rollups later. Please update us with an ETA for this fix.

I reached out on Twitter and GitHub on multiple occasions to try and get some break in the silence. It took 4.5 months for you to respond the first time. (What was worse, no notification email was sent- I typically get them, and I checked spam- so I had no way to know you had responded till I checked a couple weeks later.) Now it's been another 7.5 months of silence since another person and I asked when the fix is rolling out. This is a fairly critical bug which has been afflicting our development and production code with real customers.

It won't take much to restore good faith. Please understand that silence is not amusing.

@terrajobst instructed me to move this from https://connect.microsoft.com/VisualStudio/feedback/details/2592987 to GitHub so that he can point the correct people to it. Thanks Immo!


Posted by @jnm2 on 4/15/2016

Edit: because the attachments I'm uploading are not showing up, here is Program.cs: https://gist.github.com/jnm2/a5ab5c49e3ec4525367229f0e94e6b31

Edit 2: Because people were confused about the finalizer thread, here's a WinForms app: https://github.com/jnm2/misc-codesamples/blob/master/Bug%20reports/.NET/SqlDataReader.NextResultAsync%20bug.zip?raw=true

Edit 3: The same defect plagues ReadAsync! https://github.com/jnm2/misc-codesamples/blob/master/Bug%20reports/.NET/SqlDataReader.ReadAsync%20bug.zip?raw=true

There are two stages to this bug.

In stage 1, SqlDataReader.NextResultAsync does not handle a SqlException properly. It leaves a second internal Task with no references, available to garbage collection, whose exception has not been observed even though NextResultAsync is being awaited properly. Failure is imminent.

In stage 2, the application causes a GC. In practice this is via loading new windows and data, but to be more deterministic about it the sample uses GC.Collect() or while (true) await Task.Yield(); All three of these are equivalent; the sole purpose is to cause a GC. When the GC happens, it detects and collects the second internal Task object that NextResultAsync created and runs the finalizer which throws the task's exception via TaskSchedule.UnobservedTaskException.

I hypothesize, looking at the reference source, that the async retry code makes a mistake as it juggles Task objects. But you would know best.

=============================================

SqlDataReader.NextResultAsync is causing unobserved task exceptions when it is properly awaited:

using (var command = new SqlCommand("select null; select * from dbo.NonexistentTable;", connection))
{
    try
    {
        using (var reader = await command.ExecuteReaderAsync()) // ExecuteReaderAsync does not exhibit the bug, so we have the first result set cause no problems
            while (await reader.NextResultAsync()) { } // NextResultAsync will fail and exhibit the bug
    }
    catch (SqlException ex)
    {
        // ex is definitely observed by the await and caught, but it will show up in TaskScheduler.UnobservedTaskException as though we hadn't handled it
    }
}

Complete repro attached.

The impact is that instead of expected errors being handled and quietly logged, they trigger our application's unexpected error recovery mode. It's a valid assumption that if there is a real unobserved exception, the error should be reported and the software should be shut down.

With SqlDataReader.NextResultAsync acting this way, our application can't tell the difference between expected SqlExceptions and critical SqlExceptions. (That is, handled exceptions and unhandled exceptions.)

I'm running Windows 10 x64 and .NET 4.6.1.


Posted by Microsoft on 4/15/2016

Thank you for your feedback, we are currently reviewing the issue you have submitted. If you require immediate assistance with this issue, please contact product support at http://support.microsoft.com/oas/default.aspx?prid=15694.


Posted by @jnm2 on 4/19/2016

Guys, I've uploaded Program.cs with no errors multiple times but it's not showing up under attachments. Why not?


Posted by @jnm2 on 4/19/2016

I added github links for the samples. The multiple trial attachments should be deleted.

Also added more information to clarify what's happening. Hopefully this makes it easier to focus on the core facts.


Posted by @jnm2 on 8/9/2016

I discovered the same fault in ReadAsync. Updated. Also, it's been a few months. 4.6.2 has come and gone. Please get back to me about this.


Posted by @corivera on 8/24/2016

The fix should be available with the next batch of .NET fixes in October.


Posted by @jnm2 on 9/7/2016

That is good news! Thank you! It would have helped us immensely to receive some type of acknowledgment sooner than 4 months so that we'd know how to plan around this, but better late than never.

Does the fix include ReadAsync as well? I assume so, but I need to double check.


Posted by @corivera on 9/9/2016

Yes, the fix also affects ReadAsync. The bug was in an underlying method called by both functions.


Posted by @jnm2 on 9/9/2016

What do we look for to know when the fix is available to our customers? A Windows Update KB#?


Posted by @corivera on 9/12/2016

Yes, this will be released as a Windows .NET hotfix.


Posted by @corivera on 9/12/2016

Correction, this will be released as part of a general Windows .NET update, not a specific hotfix for this one issue.


Posted by @corivera on 9/28/2016

Update: The fix for this issue will not be included in the .NET October update. Instead, the fix will be included in the next version update of .NET framework.


Posted by Frode G on 10/30/2016

When will the next version update of .NET framework be? This bug affects our application as well. We are catching unobserved exceptions now and sets as observed. Hope that prevents IIS recycles. Looking forward for a fix.


Posted by @jnm2 on 12/14/2016

It's now been eight months, one new version of .NET and three monthly rollups later. Please update us with an ETA for this fix.

@corivera

This comment has been minimized.

Member

corivera commented Jun 13, 2017

Does this issue still repro with .NET 4.7?

The Core change for this fix can be seen in this commit here: dotnet/corefx@d44b530#diff-c0d3a7cb7fd5fe01f9f9286ed048e50f

The 4.7 reference source also has that fix present, as seen in the code here: http://referencesource.microsoft.com/#System.Data/System/Data/SqlClient/SqlDataReader.cs,4631

@corivera

This comment has been minimized.

Member

corivera commented Jun 13, 2017

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jun 13, 2017

@corivera It appears to be fixed in 4.7. Let me start a VM so I can check on 4.6.2 which is what our customers have installed.

@corivera

This comment has been minimized.

Member

corivera commented Jun 13, 2017

The fix is currently only available in 4.7. We haven't done any downlevel ports for this fix.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jun 13, 2017

Gotcha. A downlevel fix is exactly what my company is going to want.

@corivera

This comment has been minimized.

Member

corivera commented Jun 13, 2017

Do you just need this ported to 4.6.x versions?

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jun 13, 2017

Yes, although if you had asked a few months ago, I think the answer would have been 4.5.x as well.
Fwiw I can't imagine folks on 4.5.x complaining that this was fixed. I first diagnosed this on 4.6.1 though, so it's also possible the bug didn't exist prior to that. I don't know.

@corivera

This comment has been minimized.

Member

corivera commented Jun 13, 2017

If it's also present in other supported versions, then we'll fix it at the same time.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jun 13, 2017

Thank you. This is much appreciated.

@corivera

This comment has been minimized.

Member

corivera commented Jun 15, 2017

Small update, we'll only be porting this fix to 4.6.x versions, since we haven't had any requests for a fix in 4.5.2.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jul 25, 2017

Can you give us a sense of when this fix will land? I see other ADO stuff in https://blogs.msdn.microsoft.com/dotnet/2017/07/24/net-framework-july-2017-preview-of-quality-rollup/.

@corivera

This comment has been minimized.

Member

corivera commented Jul 27, 2017

@jnm2 The fix for this issue is part of that July preview rollup, although it isn't listed as part of the Data fixes.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Jul 27, 2017

Terrific!

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

@corivera All the July updates I've seen are Windows 10 only. We need this on both 7 and 10. I installed Windows 7 in a virtual machine, ran all the updates possible including the August rollup, and the bug still repros. Why is there so much Windows 10-only stuff and how long will it be until the 4.6.x fix is available on Windows 7?

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

This fix should be available as part of those July optional updates on the rollup page. I'll look into this.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

No July rollup update has shown up for the Windows 7 virtual machine. These are the only updates that contain the word "rollup":

  • 2017-08 Security Monthly Quality Rollup for Windows 7 for x64-based Systems (KB4034664)
  • May, 2017 Security and Quality Rollup for .NET Framework 3.5.1, 4.5.2, 4.6, 4.6.1, 4.6.2 on Windows 7 and Server 2008 R2 for x64 (KB4019112)

It's saying "there are no updates available for your computer" and the bug still repros.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

@corivera Am I mistaken in thinking that the fact that the August rollup is released means that the July rollup is out of preview and released via Windows Update?

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

Can you still repro the issue when installing the following update manually? This is one of the updates listed on the July rollup page.

https://support.microsoft.com/en-us/help/4024848/july-2017-description-of-preview-of-quality-rollup-for-the-net-framewo

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Which of these files do I download (https://www.catalog.update.microsoft.com/Search.aspx?q=4032113, top link)?

ndp46-kb4024848-x86_a11785673460f4908f6228186a8b4854ed615c5b.exe
ndp45-kb4024845-x86_f730026c53922cd1ddc7abb0b8e9e2def0cc6fc8.exe
windows6.1-kb4019990-x86_1365fb557d5e5917cbf59b507eac066ad89ea3f7.msu
windows6.1-kb4014596-x86_6cf907ea76be85305e767b0ea42559e48c772f70.msu

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

kb4024848 is the 4.6+ Win7 update, so the first one

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Ran it, getting:

Software Update KB4024848 Installation Wizard does not apply, or is blocked by another condition on your computer.

and no further info.

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

Does it persist after a restart?

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Yes.

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

Do the x64 updates work?

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Oh 😳 I didn't see the x86. The VM is x64.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

Great! Are we good to close this issue then?

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

My question still is, why isn't the bug fixed via recommended Windows Update? Is the fix still in preview?
Was the fix pulled from the July rollup which I assume must be rolled up inside the August rollup?

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

Was that August rollup part of the windows update, or did you have to install it manually also?

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

The July .NET preview rollup shows up for me in the optional updates list. The only August rollup I'm seeing is for Windows, not .NET. What was the KB# of the august rollup you installed?

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Was that August rollup part of the windows update, or did you have to install it manually also?

It was automatic. The only manual install I did was the one you asked just now.

The July preview rollup shows up for me in the optional updates list.

What could be different?

image

Either way we don't want to have to tell our customers to look for an optional update to keep our software stable on each machine. Things should just work for them (and us...)

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

That update screenshot says you're receiving updates for Windows only. Can you get the .NET optional updates to show up after following the instructions in this article? https://technet.microsoft.com/en-us/library/ff642466.aspx

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 10, 2017

Started a brand new VM, and I am seeing the July preview as an optional update.

So, back to the question. When will it become an automatic recommended update?

@corivera

This comment has been minimized.

Member

corivera commented Aug 10, 2017

The rollup will be pushed broadly with the next .NET Security and Quality payload, which will likely be released in September.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Aug 11, 2017

Thank you.

@jnm2

This comment has been minimized.

Contributor

jnm2 commented Sep 22, 2017

Confirmed that the recommended update 2017-09 Security Monthly Quality Rollup for Windows 7 for x64-based Systems (KB4038777), published September 12, resolves this issue. 17 months between the initial report and the ability to safely assume the fix is on most computers.

Thanks for getting this done.

@jnm2 jnm2 closed this Sep 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment