Bug 39308 - using clause prematurely garbage collects the allocated resource
Summary: using clause prematurely garbage collects the allocated resource
Status: RESOLVED NORESPONSE
Alias: None
Product: Runtime
Classification: Mono
Component: JIT (show other bugs)
Version: unspecified
Hardware: PC Linux
: --- normal
Target Milestone: ---
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2016-03-02 20:40 UTC by Ken Birman
Modified: 2017-10-11 17:21 UTC (History)
3 users (show)

See Also:
Tags:
Is this bug a regression?: ---
Last known good build:


Attachments

Description Ken Birman 2016-03-02 20:40:52 UTC
I fixed a bug in my Vsync library (used to be called Isis2), that centers on a disagreement between me and Mono C# over the semantics of the using clause. In Vsync, I have lock-wrappers that bump the thread priority to avoid priority inversions (where a high priority thread could be waiting on a lock held by a low priority thread). The code looks like this:

       using(new LockAndElevate(myLock)) { ... code protected by myLock ... }

My understanding is that this is exactly equivalent to

       try { resource = new LockAndElevate(myLock)); ... code ... } finally { resource.Dispose(); }

But in fact the mono garbage collector seems to conclude that there are no references to the locked object in my protected code (the first example where there is no variable used), and garbage collects it while still in the "protected" code block. This behavior goes away when I change my code to:

       using(var tmp = new LockAndElevate(myLock)) { ... code ... }

So here by adding a variable that I never reference, I prevent premature GC. But semantically, the C# compiler should be able to do a code analysis and realize that the tmp variable is not referenced, delete the "var tmp = " portion, at which point it might again incorrectly collect my wrapped lock prematurely. Thus my fix worries me because a future compiler improvement to Mono could again break this logic.

FAQ: 

[1] The C# spec is pretty clear in that using (resource) statement will be translated to (in short) resource; try { statement } finally { resource.Dispose(); }, meaning resource should not be disposed during the execution of statement, let alone be garbage collected.
[2] The C# 5 spec states that resource-acquisition within a using statement requires local variable(s) to be defined.  Here I'll argue that this is a meaningless requirement.  By defining a variable I did fix the issue, but in fact a dead code analysis would notice that my tmp variable isn't used, and remove it, back to case 1.  Even if I added fake code to use it, someday your dead code logic will be smart enough to detect that my code is fake.  So this leads to a future optimization that removes the dead code, etc, and we are back to case 1 again.  In effect, "using" would do nothing.
[3] How do I know this is the problem?  In fact, we worked this out extremely painfully. Vsync was throwing null pointer exceptions on these myLock objects, yet the objects themselves are static fields of a class that was long ago loaded, and were initialized by static initializers (for example, static var myLock = new VsyncLock(...)). We were baffled: how could they become null? Eventually we realized that wrapper was being disposed and the references it had to the myLock object were nulled. Then pinned this down to see that it happened while in the code block. Very hard to trigger! 
[4] Do I have a minimal, complete and verifiable example?  Unfortunately, no: I tried simple things and can't understand precisely how to trigger this.  The bug, though, was reproducible in Vsync and we saw it hundreds of times, only on Mono, running identical applications that worked on .NET.  Adding this extra declaration eliminated the behavior.  I did have a theory that perhaps I could launch a bunch of threads, all with recursive calls to using, pile up a lot of such objects, and then use the explicit garbage collector activation to see if I could force the GC into action just at that moment.  But I don't understand the implementation well enough to generate the proper scenario.  The Vsync case was seen mostly with a particular application, so while we reproduced it LOTS of times, you won't find that useful.
[5] What's Vsync?  New name for the Isis2 library for distributed computing (vsync.codeplex.com), about five years old now and rather mature, used in the smart power grid and for other mission-critical computing tasks.  This was the first bug I've actually seen in more than 14 months (did you improve the dead code logic in the compiler?  If so, perhaps that's what triggered me seeing this for the first time now).
Comment 1 Ken Birman 2016-03-02 20:45:15 UTC
PS: Eric Lippert (C# expert on SO) thinks I should perhaps just replace the 972 uses of using in my code with

     var resource = new LockAndElevate(myLock);
     try { code } finally { resource.Dispose(); }

I may do this; it would eliminate any question of the proper compilation for a using clause with no variable declared, or where the declaration looks like dead code.  But this is ugly compared to the code I currently work with.
Comment 2 Ken Birman 2016-03-02 20:56:19 UTC
PPS: Turns out that in ASP.Net they use the exact same construct as me, according to @CodeCaster on SO: 

... ASP.NET MVC team, the source code is full of this:

     using (@Html.BeginForm()) { ... }, 

where MvcForm.Dispose() will print </form>...
Comment 3 Marek Safar 2016-03-04 15:48:45 UTC
This is not C# compiler issue (at least not from what you wrote about it).

You can write simple code like

using System;

class LockAndElevate : IDisposable
{
	public LockAndElevate (object mlock)
	{
	}

	public void Dispose ()
	{
		Console.WriteLine ("dispose");
	}
}

class X
{
	public static void Main ()
	{
		object myLock = null;
		using(new LockAndElevate(myLock))
		{
			Console.WriteLine ("locked");
		}		
	}
}

and it does what you said. I am not saying there is not mono bug running your code but unless there is way for us to reproduce it. It'll hard to know for sure.

Please provide steps how to reproduce your issue.

Moving to runtime for now as it could be regalloc issue
Comment 4 Ken Birman 2016-03-04 16:03:56 UTC
Yes, this is the kind of logic used in my code -- precisely how I intended that it work.  I've observed "Dispose" to be executed while I'm still in the using block, and highly reproducibly too.

The issue does not arise if I define a temporary variable that names the resource allocated in the using block.

I presume that this is a GC bug, and agree that it should probably be in runtime.  Sorry not to have a minimal example.  Fully appreciate that lacking one makes it harder for you.  We see it very reproducibly with my software library in one particular, kind of high-stress situation, only on Mono, and only when the resource reference isn't saved into a temporary variable.  But I completely understand why you wouldn't feel that this is a fair way to report this kind of subtle bug.

If I understood the implementation of the Mono runtime better, I could probably give you a minimal example provoking this, given that I do know how to provoke it and also how to prevent it from happening in my code...
Comment 5 Ken Birman 2016-03-04 16:08:53 UTC
(PS: the sense in which it might be a compiler bug is that the compiler should probably be allocating a temporary variable to hold the reference while the using block is active, and then the GC would count the reference.  So the fix is probably a small change to the compiler).
Comment 6 Rodrigo Kumpera 2017-10-11 17:21:48 UTC
If your constructor throws, Dispose won't be called, because the compiler translates to this:

tmp = new LockAndElevate ();
try {
 ...
} finally {
  if (tmp != null) tmp.Dispose ();
}


I verified codegen on OSX amd64 and it correctly generates GC tracking of the object.

Please provide a full test case and environment you're trying it so we can reproduce your issue.

Note You need to log in before you can comment on or make changes to this bug.