kwaegel's picture

[Solved] AccessViolationException in Cloo

[EDIT] This problem was fixed by updating to the latest version of nVidia's graphics driver. Works with version 263.06 and later.

I have been beating my head against some AccessViolationExceptions in Cloo for some months now, and I finally think I found the cause. The new code nythrix put in showing what threads were interacting with the Cloo objects was the tip off. Thanks nythrix!

This error appears to occur when ComputeResource objects constructed with a OpenCL/GL shared context are freed by a different thread then they were created by. In the cases I have tested, this appears to be the system cleanup thread, not the OpenTK thread calling Game.Dispose() . The latter thread is the safe thread to use.

As near as I can tell with my general lack of knowledge of OpenGL, it looks like only the main thread has a valid OpenGL context. The other threads do not and thus are not allowed to access (i.e. free) the shared blocks of memory.

I have included some code showing a test case below. This is a very stripped down version of another testing program I have written, and thus does not really do anything. To cause this error, either the second or third line in the Game.Dispose() function should be commented out. To fix this error in larger programs, I think that every ComputeResource object needs to be disposed of by the same thread that created it.

// AccessViolationException test case.
namespace ClooTest
{
    using System;
    using System.Drawing;
    using System.Threading;
    using System.Diagnostics;
    using System.Collections.Generic;
    using System.Runtime.InteropServices;
 
    using OpenTK;
    using OpenTK.Graphics;
    using OpenTK.Graphics.OpenGL;
    using OpenTK.Input;
 
    using Cloo;
 
    class SimpGame : OpenTK.GameWindow
	{
		// required for OpenCL-OpenGL interop
		[DllImport("opengl32.dll")]
		extern static IntPtr wglGetCurrentDC();
 
		OpenTK.Graphics.IGraphicsContextInternal _glContext;
 
		ComputeContext _computeContext;
		ComputeCommandQueue _commandQueue;
 
        /// <summary>Creates a window with the specified title.</summary>
        public SimpGame()
            : base(400, 400, GraphicsMode.Default, "Cloo shared context tester")
        {
            VSync = VSyncMode.On;
        }
 
        protected override void OnLoad(EventArgs e)
        {
            base.OnLoad(e);
 
			GL.ClearColor(Color.Black);
 
			openCLSharedInit();
 
            // Do not need to build a program or kernel to reproduce error.
        }
 
		// Create a sharde context between OpenGL and OpenCL. 
		private void openCLSharedInit()
		{
			// select OpenCL device and platform. Need GPU for shared context.
			ComputePlatform platform = ComputePlatform.Platforms[0];
			ComputeDevice device = platform.Devices[0];
            if (device.Type != ComputeDeviceTypes.Gpu)
            {
                platform = ComputePlatform.Platforms[1];
                device = platform.Devices[0];
            }
 
            Trace.WriteLine("Creating shared context on "+ device.ToString());
 
 
			IntPtr curDC = wglGetCurrentDC();
 
			_glContext = (OpenTK.Graphics.IGraphicsContextInternal)OpenTK.Graphics.GraphicsContext.CurrentContext;
			IntPtr raw_context_handle = _glContext.Context.Handle;
			ComputeContextProperty p1 = new ComputeContextProperty(ComputeContextPropertyName.CL_GL_CONTEXT_KHR, raw_context_handle);
			ComputeContextProperty p2 = new ComputeContextProperty(ComputeContextPropertyName.CL_WGL_HDC_KHR, curDC);
			ComputeContextProperty p3 = new ComputeContextProperty(ComputeContextPropertyName.Platform, platform.Handle);
			List<ComputeContextProperty> props = new List<ComputeContextProperty>() { p1, p2, p3 };
			ComputeContextPropertyList Properties = new ComputeContextPropertyList(props);
 
			_computeContext = new ComputeContext(device.Type, Properties, null, IntPtr.Zero);
 
			//Create the command queue from the context and device
			_commandQueue = new ComputeCommandQueue(_computeContext, device, ComputeCommandQueueFlags.None);
		}
 
 
        protected override void OnUpdateFrame(FrameEventArgs e)
        {
            base.OnUpdateFrame(e);
            if (Keyboard[Key.Escape]) { Exit(); }
        }
 
        protected override void OnRenderFrame(FrameEventArgs e)
        {
            base.OnRenderFrame(e);
        }
 
 
		public override void Dispose()
		{
			Trace.WriteLine("Dispose called in thread(" + Thread.CurrentThread.ManagedThreadId+")");
 
			// Ensure all OpenCL objects are disposed of in the main thread.
			// WARNING: Removeing any of these dispose lines will cause an AccessViolationException on program shutdown.
 
			//_commandQueue.Dispose();
			_computeContext.Dispose();
 
			base.Dispose();
 
		}
 
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        static void Main()
        {
            System.Diagnostics.Trace.WriteLine("\n********** Run at " + System.DateTime.Now.ToString() + " **********");
 
			// The 'using' idiom guarantees proper resource cleanup.
			// We request 30 UpdateFrame events per second, and unlimited
			// RenderFrame events (as fast as the computer can handle).
			using (SimpGame game = new SimpGame())
			{
				game.Run(30.0);
				//game.Run();
			}
        }
    }
}

Comments

Comment viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
nythrix's picture

Indeed, GC cleanup occurs in a different thread which is my main suspect. That's the reason I added those trace prints. Unfortunately there's not many info on this in the OpenCL specs nor how do shared contexts behave in these situations.

The problem is this code:

        public override void Dispose()
        {
            Trace.WriteLine("Dispose called in thread(" + Thread.CurrentThread.ManagedThreadId + ")");
 
            // Ensure all OpenCL objects are disposed of in the main thread.
            // WARNING: Removeing any of these dispose lines will cause an AccessViolationException on program shutdown.
 
            _commandQueue.Dispose();
            _computeContext.Dispose();
 
            base.Dispose();
        }
 
        /// <summary>
        /// The main entry point for the application.
        /// </summary>
        static void Main()
        {
            System.Diagnostics.Trace.WriteLine("\n********** Run at " + System.DateTime.Now.ToString() + " **********");
 
            // The 'using' idiom guarantees proper resource cleanup.
            // We request 30 UpdateFrame events per second, and unlimited
            // RenderFrame events (as fast as the computer can handle).
            using (SimpGame game = new SimpGame())
            {
                game.Run(30.0);
                //game.Run();
 
                //game.Dispose();
 
                Thread t = new Thread(new ThreadStart(game.Dispose));
                t.Start();
                t.Join();
                /* */
            }
        }

doesn't trigger the exception. And that's where I'm currently stuck. But I still believe we're on a good track.

c2woody's picture

Works fine here on some nvidia card with the 266.xx drivers. The Dispose in the non-GUI thread can be traced and does not trigger an exception.

kwaegel's picture

That is quite odd. Calling dispose works in any thread except the GC? Is there something special about the GC thread?

I was looking through the OpenTK code and noticed the liberal use of GC.SuppressFinalize(), so I decided to try that and see what happened. Interestingly enough, code below terminates without errors:

public override void Dispose()
		{
			Trace.WriteLine("Dispose called in thread(" + Thread.CurrentThread.ManagedThreadId+")");
 
			//_commandQueue.Dispose();
			//_computeContext.Dispose();
			GC.SuppressFinalize(_commandQueue);
			GC.SuppressFinalize(_computeContext);
 
			base.Dispose();
		}

So the error is clearly thrown when the GC system calls Finalize().

As an interesting read, in the last post in this thread someone says that when are handling unmanaged resources, not calling dispose could be considered a programing error (on the part of the library user). That does seems to defeat the purpose of using a managed language, though.

One workaround might be to maintain a static collection of ComputeResources that can all be disposed with a single call at program termination.

nythrix's picture

That code looks ok because the context and the command queue aren't released until the program terminates. Then the drivers wipe out the leftovers so it sort of works. Quite nasty though.
I don't like the idea of manual cleanup either. For the same reason you've mentioned.
Pooling ComputeResources is an option. However, it will force most of the objects to live until static ComputeResource.DisposeAll() is called. Which isn't a brilliant situation. But it is better than crashing.
I'll take any advice on this.

nythrix's picture

c2woody: which code did u run? The one expected to crash?

vicviper's picture

Implementing IDisposable can become quite tricky, and it's a common cause of problems to most developers... I tend to avoid them as much as possible.

here's a quite complete explanation, everybody doing unmanaged bindings should read this: http://www.codeproject.com/KB/dotnet/IDisposable.aspx

The basic idea is that (at least what I think I understood) is that, when the finalizer calls your Dispose, your object might be partially destroyed; that means that an internal reference to another managed object may or may not be valid, it is indeed a very chaotic and hard to debug behavior.

for example:

class foo : IDisposable
{
private bar _ManagedResource;
private IntPtr _UnmanagedResource;
 
public Dispose()
{
ManagedResource.Dispose();  // if called from the code, this is ok, if called from the finalizer, this might fail because ManagedResource can be destroyed already
 
UnmanagedInvokeDestroy(_UnmanagedResource);
 
}
 
}

In general, I try to avoid complex classes that mix managed and unmanaged code, also, I try to avoid finalizers, and rely exclusively on enforcing developers to call the Dispose manually.

what I usually do is this:

class foo : IDisposable
{
 
 
private bool _Disposed = false;
 
public Dispose()
{
// do whatever you need to do here
 
_Disposed  = true;
}
 
#if DEBUG
 
~foo()
{
System.Diagnostics.Debug.Assert(_Disposed,"did you forgot to call Dispose?");
}
 
#endif
 
}

This way, if I forget to call Dispose, I get an error at runtime when debugging.

This method is also more efficient, because the GC typically requires two cycles to remove a class with a finalizer, and classes with finalizers tell bad programmers: "oh, I can forget calling dispose" , so I think it's better to warn developers when they forgot to call Dispose, than to let them think they can rely on finalizers.

c2woody's picture
Quote:

c2woody: which code did u run? The one expected to crash?

The one in the original posting, yes.

kwaegel's picture

Have you considered having ComputeObject extend SafeHandle instead of storing an IntPtr handle directly? I just read this MSDN blog article which seems to say that doing so is the preferred solution.

(Here are three more articles discussing SafeHandles.)

If I understand correctly, SafeHandles wrap a IntPtr and safely implements the finalization and cleanup methods.

One thing I am unsure about is that the above articles are several years old (2006 and older), so I am not sure if the preferred practices have changed since then. Also, most of the articles refer to the use of SafeHandle in server code and Win32 handles, so I am not sure if it is really intended to work with something like OpenCL.

nythrix's picture

I had already read on finalization and different techniques in this area when started implementing Cloo. I was somehow familiar with SafeHandle and others. When doing ComputeObject I went with the classic approach because there was no reason for adding complexity (i.e.: it worked so I KISSed myself). Of course, back then I didn't know that shared objects behave differently.

I've been reading the VERY THOROUGH article that vicviper linked to. I'm not sure if I come up with something but it doesn't look good. Fiddler would also go for manual disposing of unmanaged resources (linky).

Edit: I finally have some free time during the next days so hopefully I'll be able to put something together. Either a fix or a workaround comment.
Edit2: SafeHandle doesn't really help here. Some of the problems it solves cannot occur with Cloo because it operates on a slightly higher level (most of the problems have already been sorted out: kernel arguments and execution, async CommandQueue methods). I don't know about handle recycling. AFAIK that doesn't happen in OpenCL (not sure though).

vicviper's picture

My suggestion would be to call Dispose manually, and have finalizers only in debug for runtime developer warnings. These are my thoughts:

- Microsoft says the good practice is to always call Dispose, whenever a class implements IDisposable, no exceptions.
- If Dispose must always be called, then, why have finalizers anyway?
- My guess is that, in the first days of .NET beta testing, developers forgot too often to call Dispose, and since unmanaged resources are critical, this produced NET applications that could easily break the operating system, this wasn't a good thing for microsoft, since they wanted to show NET as a new, good, and reliable technology, not something that crashed windows after 5 minutes of running.
- So at that point, microsoft decided to introduce finalizers, as a neccesary evil, to safe the system against bad programmers.

So that's what I think about finalizers, they're there only for bad programmers, because a good programmer should call Dispose, and by doing so, finalizers would never end being called.

- personal opinion below this line -

My conclusion is that we must encourage developers to call Dispose (good practice) instead of let them rely on finalizers (bad practice) .

Also, given that relying on finalizers is bad practice, it's also a bad practice to expend too much time implementing bad finalizers that encourage bad programmers, so in my code, the only finalizers I implement are the debug only runtime warnings, they are very easy to implement, encourage good practices (to call dispose) and disappear in production code.

As a final note, I'll say I don't like this, I come from C++, and I would love to have C++ like destructors in NET, but the way finalizers have been implemented in NET, they're almost useless for most tasks, so it's better to fall back to Dispose().