C# running background worker executes twice
I am trying to use background worker for the first time. The problem I am having is that the code within the background worker code appears to run twice. I start my code with this.
public frmImportPayrollUtility()
{
InitializeComponent();
bgwBank.DoWork += bgwBank_DoWork;
bgwBank.RunWorkerCompleted += bgwBank_RunWorkerCompleted;
bgwBank.WorkerReportsProgress = false;
bgwBank.WorkerSupportsCancellation = false;
}
============================================================
My click event is this
private void btnBankTrx_Click(object sender, EventArgs e)
{
lblBankTrxs.Text = "Begin Importing Bank Transactions"; /*
bgwBank.RunWorkerAsync();
}
==============================================================
My background worker events are
private void bgwBank_DoWork(object sender, DoWorkEventArgs e)
{
CreateBankChecks();
CreateOtherAdjustments();
FlipWasImportedFlag();
UpdateBankImportDate();
}
private void bgwBank_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
{
lblBankTrxs.Text = "Importing Bank Transactions Finished";
MessageBox.Show("Transactions successfully imported into Bank Transactions");
}
public frmImportPayrollUtility()
{
InitializeComponent();
bgwBank.DoWork += bgwBank_DoWork;
bgwBank.RunWorkerCompleted += bgwBank_RunWorkerCompleted;
bgwBank.WorkerReportsProgress = false;
bgwBank.WorkerSupportsCancellation = false;
}
============================================================
My click event is this
private void btnBankTrx_Click(object sender, EventArgs e)
{
lblBankTrxs.Text = "Begin Importing Bank Transactions"; /*
bgwBank.RunWorkerAsync();
}
==============================================================
My background worker events are
private void bgwBank_DoWork(object sender, DoWorkEventArgs e)
{
CreateBankChecks();
CreateOtherAdjustments();
FlipWasImportedFlag();
UpdateBankImportDate();
}
private void bgwBank_RunWorkerCompleted(object sender, RunWorkerCompletedEventArgs e)
{
lblBankTrxs.Text = "Importing Bank Transactions Finished";
MessageBox.Show("Transactions successfully imported into Bank Transactions");
}
ASKER CERTIFIED SOLUTION
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
The change to multithreading now allows your app to be responsive when running heavy processing tasks. Yippee.
The change to multithreading now exposes a downside you probably haven't considered - it allows your app to be responsive when running heavy processing tasks. Huh !
No app freeze = the work has been done:- so why haven't I got the results.
Some sort of visual indication that the app is working is often required. Labels being shown, things becoming disabled, cursor changes......
It means that now a button event will be fired twice if the mouse has a hardware problem or the user clicks a second time whilst waiting for example. (I'm not going to talk about users suddenly losing the ability to read selectively when using a computer). You usually require to have protective code in place to stop (or at least hide) the results of that multiple event handling from the user. Disabling the button/menu is an obvious choice whilst the thread is running.
The change to multithreading now exposes a downside you probably haven't considered - it allows your app to be responsive when running heavy processing tasks. Huh !
No app freeze = the work has been done:- so why haven't I got the results.
Some sort of visual indication that the app is working is often required. Labels being shown, things becoming disabled, cursor changes......
It means that now a button event will be fired twice if the mouse has a hardware problem or the user clicks a second time whilst waiting for example. (I'm not going to talk about users suddenly losing the ability to read selectively when using a computer). You usually require to have protective code in place to stop (or at least hide) the results of that multiple event handling from the user. Disabling the button/menu is an obvious choice whilst the thread is running.
As already posted, a background worker can be executed "more than" once, cause you now can easily double-click your button. In scenarios, where you the offloaded payload must be run only once at a time you need a kind of protection.
The first level is to disable the button or command, so that the UI signals that this task is not available.
The second level is to programmatically prevent it from running twice at a time by using a semaphore, mutex or .NET native a lock.
The first level is to disable the button or command, so that the UI signals that this task is not available.
The second level is to programmatically prevent it from running twice at a time by using a semaphore, mutex or .NET native a lock.
ASKER
Maybe I should have my button wear a mask? I think gr8gonzo is on to something. I do currently have defined as static. I will switch to local and see how it goes. Where is the button to attach a file. I want to provide the complete code.
ASKER
I see the insert file button. Here is the code.frmImportPayrollUtility.cs
Just some comments:
- Get as verb must be only used for getter methods.
- The import code is a classic example for a missed OO chance. The import processing should be its own class. Communication with the invoking instance should happen by events (no internal message boxes). Benefit: You can write a unit test for the importer.
- I guess Model is injected via Dexterity? In conjunction with the direct SQL and the DataAccess layer, it smells like an additional controller would be useful for development, debugging and maintenance.
- Your usage of bgwflag allows the background worker to execute multiple times in parallel. Use the lock statement instead.
- As you're using Dexterity, I would also dig into the documentation and read about what it it offers of transactional control, maybe this is the option to go instead or in addition to lock() to prevent parallel execution by different users.
- Get as verb must be only used for getter methods.
- The import code is a classic example for a missed OO chance. The import processing should be its own class. Communication with the invoking instance should happen by events (no internal message boxes). Benefit: You can write a unit test for the importer.
- I guess Model is injected via Dexterity? In conjunction with the direct SQL and the DataAccess layer, it smells like an additional controller would be useful for development, debugging and maintenance.
- Your usage of bgwflag allows the background worker to execute multiple times in parallel. Use the lock statement instead.
- As you're using Dexterity, I would also dig into the documentation and read about what it it offers of transactional control, maybe this is the option to go instead or in addition to lock() to prevent parallel execution by different users.
private void btnBankTrx_Click(object sender, EventArgs e)
{
btnBankTrx.Enabled = false;
this.Refresh;
// or this.Invalidate;
bgwBank.RunWorkerAsync();
}ASKER
Following the advice of gr8gonzo I changed bgwBank to local and the code only executes once. However I see I still have much to learn. I will speak with one of my Dexterity gurus and run this idea past them.
private void btnBankTrx_Click(object sender, EventArgs e)
{
BackgroundWorker bgwBank = new BackgroundWorker();
bgwBank.DoWork += bgwBank_DoWork;
bgwBank.RunWorkerCompleted += bgwBank_RunWorkerCompleted;
bgwBank.WorkerReportsProgress = false;
bgwBank.WorkerSupportsCancellation = false; //Do not allow for the process to be cancelled
bgwBank.RunWorkerAsync();
}
private void btnBankTrx_Click(object sender, EventArgs e)
{
BackgroundWorker bgwBank = new BackgroundWorker();
bgwBank.DoWork += bgwBank_DoWork;
bgwBank.RunWorkerCompleted += bgwBank_RunWorkerCompleted;
bgwBank.WorkerReportsProgress = false;
bgwBank.WorkerSupportsCancellation = false; //Do not allow for the process to be cancelled
bgwBank.RunWorkerAsync();
}
bgwBank.WorkerSupportsCancellation = false; //Do not allow for the process to be cancelled
This property is false by default according to the doc, so removing it would make the code cleaner. I think the same could be done to the WorkerReportsProgress line too. Besides that, if you're not checking the cancellation token inside the event handler worker or even calling Cancel method, then it makes it even more useless to even set it to false manually.
ASKER
Good point. I am still learning. Thanks for the tip.
However leaving those settings does show anyone down the line what you wanted to do (or not do) when the code has to be maintained by someone else in the future
Also, I'd suggest trying out the async/await pattern. It's a ton simpler to use and easier to do multiple async tasks at once.
Check out my article on the topic:
https://www.experts-exchange.com/articles/35473/Async-and-Multi-Threading-in-C-in-Plain-English.html
Check out my article on the topic:
https://www.experts-exchange.com/articles/35473/Async-and-Multi-Threading-in-C-in-Plain-English.html
Please show us complete code.
Edit: Another important thing to mention, in the DoWork you'd might not be applying thread-safety constraints. Which means, the code might run into multiple executions concurrently, this is just a guess and might not even be the case but if you'd wrap the logic of shared memory (variables) with a lock statement, that might be a solution to your problem.