Enforcing single thread in MFC ActiveX using Mutex

I have to make thread safety for my MFC ActiveX graphics engine because Windows 7 allow an application to run with a messagebox waiting for the user and even allowing the messagebox to be in the background while the user continue to use the program and make new calls to the engine. Without thread safety, the engine crashes with access violation.

The following mutex code did absolutely nothing and I don't understand why.

Code:

---

// Declared in public class data
HANDLE EngineMutex;

// Executed in the class constructor
EngineMutex = CreateMutex(NULL,FALSE,"EngineAccess");

// Used in start and end of all interface calls
#define START_CALL WaitForSingleObject(EngineMutex,INFINITE);
#define END_CALL ReleaseMutex(EngineMutex);

---

It seems to be better to just put all error messages in a queue and wait for the calling application to read them.

Are you protecting the right resource with your mutex?

I use the whole engine as a resource so that the caller should only be allowed to use one method at a time. My new error message system makes it safe to call from VB6 but a multicore application could still make the engine crash by having overlapping calls.

Can you post the code for a few of the methods so we can see how you are protecting them?

I use the macros START_CALL and END_CALL in each area that reads or writes using the engine's shared data but the last time I tried, more than 10 threads was in the critical section at once and caused the engine to crash in 80% of the cases.

Code:

---

void CDFPGECtrl::Camera_RenderScene(int InputCamera, int OutputSurface, int ShaderChannel) {
        START_CALL
                GET_FROM_REF(Camera,InputCamera)
                GET_FROM_REF(DrawSurface,OutputSurface)
                if BAD_REF(InputCamera) {
                        REPORT_TYPE(Camera,Camera_RenderScene,InputCamera)
                } else if BAD_REF(OutputSurface) {
                        REPORT_TYPE(DrawSurface,Camera_RenderScene,OutputSurface)
                } else if (pOutputSurface->DepthBuffer == false) {
                        MQ->InsertMessage(L"Camera_RenderScene: The draw surface reference OutputSurface does not have a depth buffer.");
                } else {
                        DGE->Camera_RenderScene(pInputCamera,pOutputSurface,ShaderChannel);
                }
        END_CALL
}

int CDFPGECtrl::Model_CreateCopy(int SourceModel) {
        int Result;
        START_CALL
                GET_FROM_REF(Model,SourceModel)
                if BAD_REF(SourceModel) {
                        REPORT_TYPE(Model,Model_CreateCopy,SourceModel)
                        Result = -1;
                } else {
                        Result = DGE->IDFromPointer(DGE->Model_DuplicateModel(pSourceModel));
                }
        END_CALL
        return Result;
}

---

What is the code for START_CALL and END_CALL?

They are declared in the first post.

Okay, so these locks are on a class instance level. This is fine if you are trying to prevent an instance of a class from calling more than one method in different threads.

If you need to synchronize across multiple class instances, but the functionality into a singleton class and just forward the method calls to the singleton class.

Something like:

Code:

---

void CDFPGECtrl::Camera_RenderScene(int InputCamera, int OutputSurface, int ShaderChannel)
{
  // forward the call to the singleton instance
  Service::GetInstance( )->Camera_RenderScene( InputCamera, OutputSurface, ShaderChannel );
}

---

Quote:

---

Originally Posted by Arjay

If you need to synchronize across multiple class instances, but the functionality into a singleton class and just forward the method calls to the singleton class.

---

I don't need synchronization between different class instances because I can run 2 instances of the engine at the same time without collisions.

Quote:

---

Originally Posted by Dawoodoz

I don't need synchronization between different class instances because I can run 2 instances of the engine at the same time without collisions.

---

If that is the case, then your previous statement appears to be incorrect: "I use the macros START_CALL and END_CALL in each area that reads or writes using the engine's shared data but the last time I tried, more than 10 threads was in the critical section at once and caused the engine to crash in 80% of the cases."

Quote:

---

Originally Posted by Arjay

If that is the case, then your previous statement appears to be incorrect: "I use the macros START_CALL and END_CALL in each area that reads or writes using the engine's shared data but the last time I tried, more than 10 threads was in the critical section at once and caused the engine to crash in 80% of the cases."

---

It appears that your engine isn't thread safe (and instances of the engine can't be called across multiple threads).

With shared data, I mean outside of the called function and not global memory.

Quote:

---

Originally Posted by Arjay

It appears that your engine isn't thread safe (and instances of the engine can't be called across multiple threads).

---

Yes, I wrote that in the first post.

Quote:

---

Originally Posted by Dawoodoz

Yes, I wrote that in the first post.

---

So put the engine into a singleton class and protect it as I've suggested.

I need multiple instances of the engine.

Quote:

---

Originally Posted by Dawoodoz

I need multiple instances of the engine.

---

If the engine itself isn't thread safe, then you aren't going to be able to use multiple instances of it from different threads within the same process. That's it - end of story.

What you can do is to wrap the engine inside an out of proc COM server (.exe server) and configure the COM server to create a new process for each engine instance.

Then have your ActiveX control create an instance of the engine via the COM server as appropriate. Not real efficient, but doable.

Of course, a better approach would be to make the engine threadsafe.

Quote:

---

Originally Posted by Arjay

If the engine itself isn't thread safe, then you aren't going to be able to use multiple instances of it within the same process.

---

I have done that and it is safe when the instances don't have any memory in common. The problem with thread safety occur when the user make two calls to the same instance.

Quote:

---

Originally Posted by Dawoodoz

I have done that and it is safe when the instances don't have any memory in common. The problem with thread safety occur when the user make two calls to the same instance.

---

You have done what? If multiple instances of the engine don't work 100% of the time, then the engine isn't thread safe. If the 'memory in common' isn't within the engine, then synchronize access to the memory.

Btw, in addition to accessing the engine with the COM class instance, are you accessing any global variables?

Multiple instances of the engine do work 100% of the time as long as the user don't make 2 calls to the same instance. I use ActiveX with no data outside of the class since sharing data between them would be stupid.

So separate instances of the engine work okay, but each engine instance isn't thread safe. Okay, that brings the question:

When do you create an engine instance? Do you create a new engine for each instance of CDFPGECtrl?

The engine is created when the control is created but a starting call is needed before the engine is running and accepting input.

Quote:

---

Originally Posted by Dawoodoz

The engine is created when the control is created but a starting call is needed before the engine is running and accepting input.

---

So when a control is created it creates a new engine instance? I'm asking because your term 'the engine' may imply only one engine instance (shared between multiple control instances).

Yes, they share absolutely nothing like a regular ActiveX component.

Put some TRACE statements in your code and spew the output to a debug window. In the TRACE statement, record the method name and the thread Id of the current thread.

That will give you an idea of if things are working as you expect. From what you are describing it sounds like an ActiveX control instance is getting shared between threads.

Yes, that is what I try to protect it from because my engine should be foolproof for beginners.

Quote:

---

Originally Posted by Dawoodoz

Yes, that is what I try to protect it from because my engine should be foolproof for beginners.

---

What could help is if you returned rich error information from your COM methods.

Why not use the COM approach and return an HRESULT

Code:

---

HRESULT CDFPGECtrl::Camera_RenderScene(...);

---

Quote:

---

Originally Posted by Arjay

What could help is if you returned rich error information from your COM methods.

Why not use the COM approach and return an HRESULT

Code:

---

HRESULT CDFPGECtrl::Camera_RenderScene(...);

---

---

Checking for return values all the time makes the engine less user friendly and I can't have multiple return values. I have a delayed message system that puts a message in a queue when an error occurs so that it is threadsafe for Visual Basic 6 but not any multicore language.

Why not do both? Provide return values for those developers that want to use them and give the option of the error messaging.

Experience developers probably want to know that a call has succeeded or not at the time of making the call.