putting critical section to work

[zspirit]

I save a file which takes a long time, the time is not an issue right now. While it is being saved, if I press save again, now there are two save operations in progress and there is obviously an assertion.

I enclosed the save into critical section so the next time user press save, it will not execute that code but this mechanism does not work.

CSingleLock singleLock(&m_cricSection);

if (singleLock.IsLocked())
{
AfxMessageBox("locked");
return;
}
singleLock.Lock()
// ---- saving
singleLock.Unlock()

I tried this with CCriticalSection object as well as CEvent, does not work. It just executes the code everytime the control is here.

How can I make this critical section to work? thanks.

[MrViggy]

That code looks correct, just like the MSDN example. Why don't you just gray out the "Save" button during the save? That has the added effect of telling the user that they cannot save yet.

Viggy

[Arjay]

I save a file which takes a long time, the time is not an issue right now. While it is being saved, if I press save again, now there are two save operations in progress and there is obviously an assertion.

I enclosed the save into critical section so the next time user press save, it will not execute that code but this mechanism does not work.

CSingleLock singleLock(&m_cricSection);

if (singleLock.IsLocked())
{
AfxMessageBox("locked");
return;
}
singleLock.Lock()
// ---- saving
singleLock.Unlock()

I tried this with CCriticalSection object as well as CEvent, does not work. It just executes the code everytime the control is here.

How can I make this critical section to work? thanks.

This will only work if you are using multi-threading. A critical section will not block if the the same thread attempts to enter the critical section more than one time.

[zspirit]

This will only work if you are using multi-threading. A critical section will not block if the the same thread attempts to enter the critical section more than one time.

Is there anything I can do in case for same thread as well? I thought CSingleLock works for single thread and CMultiLock works for multiple threads!?

MrViggy,

I already started working on that as well but had an issue that the Save button on toolbar will not respond on time, the menu option does, both have same IDs. You can have a look at it here.

[john_avi]

I can suggest you a simple idea..you can keep a bool variable which will tell you whether save process is going on or not..if it is going on then do not proceed further and return from there itself..

[usman999_1]

Or if it's your own save dialog, disable the button like most websites do.

Hope this helps,
Regards,
Usman.

[JVene]

My 2 cents worth:

First, this isn't exactly the purpose for which critical section locks are intended. It could be made to work, but critical section locks are intended to be used for short durations. They'll last as long as you like, but you're involving the OS task scheduler, it's a high speed action (especially when they're nothing else waiting), forming a queue of pending actions.

That last point means you intend that the subsequent actions, which are held up during the lock, are supposed to execute after the thread currently holding the lock releases. That's not, typically, what you want multiple save requests to do, especially if they're issued while a save is in progress. You typically want the intervening save requests to be ignored, though in some designs you MIGHT want the save to be repeated ONCE, only after the current save is completed, under the assumption that the document contents were changed while a previous save was in progress ( a dubious design notion, but perhaps it's possible).

So, you don't really want locks via critical section, you want something that BLOCKS. The notion of disabling the save button is closer to the correct design decision.

Imagine what would happen, for example, if your user hit 10 or 20 save button clicks while the first save is in progress. If you spun a thread for each save response, that document would save 20 times in a row (19 of which would probably be for no reason).

What you want is a state (previously suggested with a simple bool), that indicates a save is currently in progress. THAT boolean will need protection from a critical section (you lock the critical section before checking the boolean, if it's set to 'saving', you unlock and skip the save - otherwise you set the boolean, unlock, and save - yes - save while the critical section is unlocked, the boolean stops competing saves).

Then, don't forget to reset the boolean (under critical section lock) when the save is completed, so subsequent saves work.

Disabling the 'save' button mirrors the state of this boolean (if a save is in progress, the save button click makes no sense).

[Arjay]

Does this application use menu items for the save operation or a button?

If a button, just add a control variable for the button (in VC7.1 and above, right click on the button, choose add variable, enter a name and click finish; In VC6, use the class wizard). Next, in the save button handler call EnableWindow to disable and enable the button.

void CSaveDlg::OnBnClickedSave()
{
	// Disable the Save button
	m_btnSave.EnableWindow( FALSE );

	// Put the save code here

	// Enable the Save button
	m_btnSave.EnableWindow( TRUE );
}

It's that easy.

Btw, when you right click on the button and choose 'Add Variable' to add the control variable, a CButton variable gets added to the class declaration and the following DoDataExchange entry gets created.

void CSaveDlg::DoDataExchange(CDataExchange* pDX)
{
	CDialog::DoDataExchange(pDX);
	DDX_Control(pDX, ID_SAVE, m_btnSave);
}

[zspirit]

THAT boolean will need protection from a critical section (you lock the critical section before checking the boolean, if it's set to 'saving', you unlock and skip the save - otherwise you set the boolean, unlock, and save - yes - save while the critical section is unlocked, the boolean stops competing saves).

I agree with the rest of your comments, they are convincing but I think if there is only one therad we probably don't need to protect that bool in critical section block as well. In my like most GUI applications there is only one GUI thread - there are other worker threads but they could never call Save.

[zspirit]

void CSaveDlg::OnBnClickedSave()
{
	// Disable the Save button
	m_btnSave.EnableWindow( FALSE );

	// Put the save code here

	// Enable the Save button
	m_btnSave.EnableWindow( TRUE );
}

This is an interesting solution, but the button in my case is in toolbar. So only if there could have been anything like GetDlgItem() which could get a pointer to the button in toolbar and this would have been a good solution.

I don't think CToolBar has any member functions either which could help or is there any way one can get the button pointer from toolbar or just set the state?

[JVene]

there are other worker threads but they could never call Save

That may be true in your design, and as such, critical sections wouldn't be required at all (even when attempting to block multiple saves - only one thread could every interact with that critical section).

Now that I consider a single threaded design, it appears the only way multiple save commands COULD come through would be from a queue of the commands, meaning there would never be a lock or even a block unless you disable the save command itself (the thread would never be available to process a save button click unless the save were finished).

However, I wonder why you wouldn't place the save into a thread? It may be as well, if there's nothing your user CAN do while the save is under process, but a file save is one of the more typical reasons TO thread.

It does complicate the design some, so if there's limited benefit, then you should keep your current design. If you have an MDI or the more modern tabbed document design, or if there's anything else the user might be able to do while a save is under way, and if (if's are pilling up now) the save might take more than a second or two, saving in a thread is highly desirable. It does mean you have to postpone WM_CLOSE requests for the application, carefully handle all the related functions which can't be performed on the document itself while it's locked during saving (again a boolean state), etc. The user does appreciate the effort when it comes into play, though.

[JVene]

Sorry to double post, this is a separate concept though.

I don't think CToolBar has any member functions either which could help or is there any way one can get the button pointer from toolbar or just set the state?

If you're using standard MFC, the toolbar and menu should respond to ON_UPDATE_COMMAND_UI mechanisms.