Ouch. Well, the good news is, since we all learn best from our mistakes,
there's a lot of opportunity for learning here. :)
With respect to the code you posted, the first thing I'll note is that at
first blush, it looks basically "correct" in the sense that it looks like
it _should_ do what you want it to. It's not "correct" in the sense of
being _good_ code, but it ought to at least get the job done.
Since apparently it doesn't actually get the job done, that means there's
something to the question you still haven't told us. For example, we have
no idea what the DataTableThreader class is or does. Hopefully it's
nothing but a container for your arguments, but it might be something
else. We just don't know.
There are a number of areas in which it's more complicated to come up with
a concise-but-complete code sample. Code that involves querying a
database is one of them. However, because one can in fact create
DataTables and DataSets in memory, it's not impossible. It just takes a
little reworking. If you are unable to figure this out yourself, I'll
suggest that we _really_ need a true concise-but-complete code sample.
That is, it needs to be short, including _nothing_ that isn't directly
related to reproducing the problem. And it needs to be complete:
something that we can copy and paste into an empty file, and compile
without having to add _anything_ to produce a working executable that can
be debugged.
Without that, it's not really possible to answer the question.
Now, that said, let's look at the code you posted, and then I'll provide
an alternative that IMHO would work better (and which will show that while
you might think that BackgroundWorker wasn't useful, "daveL"'s suggestion
was actually on-point and relevant :) )...
On Mon, 25 Aug 2008 06:16:01 -0700, jp2msft
<jp*****@discussions.microsoft.comwrote:
In the main body, the DataTable is constructed with the SQL statement:
DataTable table = new DataTable("System_ID");
Global.FillTable(ref table, "EXEC AcpReport_GetSystemIDs");
It is almost certain that you should not be passing the "table" argument
by "ref". It's impossible to know for sure, since you didn't post the
code for DataTableThreader. But the "ref" keyword is for when the callee
needs to modify the original _variable_ being passed. From all
indications, you don't want your code to do that. You just want the
callee to modify the DataTable you've already allocated, not replace that
DataTable with some other one.
do {
Thread.Sleep(100);
Application.DoEvents();
} while (Global.Working == true);
The above is a big warning sign that you've got some problem. There are
two things that you practically never need to call from your own code:
Thread.Sleep() and Application.DoEvents(). You've done both.
The Thread.Sleep() method is good for two things: yielding to other
threads on purpose (in which case you pass 0 or 1, depending on which
threads you want to yield to) or delaying a thread for some very specific
amount of time, because _that time_ is important for the behavior of your
code.
Calling Thread.Sleep() with an arbitrarily selected value of 100ms is a
clear indication that you shouldn't be calling it at all.
Application.DoEvents() is just plain bad news. It creates re-entrancies
in code that almost certainly was not written with reentrancy in mind, and
it means you've co-opted the main GUI thread for something that's blocking
when it shouldn't.
foreach (DataRow row in table.Rows) { // add the generic search first
cboFilter2.Items.Add(row["System_ID"].ToString().Trim());
}
The Global FillTable method passes information to and creates the Thread:
public static void FillTable(ref DataTable table, string sqlCommand) {
if (Working == false) {
Working = true;
DataTableThreader dtParam = new DataTableThreader(ref table,
sqlCommand);
Thread dbThread = new Thread(fillTableThread);
dbThread.IsBackground = true;
dbThread.Priority = ThreadPriority.AboveNormal;
Never increase your thread priority. As with Thread.Sleep() and
Application.DoEvents(), it's practically never the right thing to do. And
it's even more dangerous than Application.DoEvents(), because thread
priorities are very important to Windows for the proper management of
resources, and doing it incorrectly can lead to the system becoming
unresponsive or, even worse, unstable.
It's especially pointless and potentially dangerous to raise the priority
of a thread that's mostly doing i/o. Such as, a thread that's querying a
SQL database.
That's about all the really important stuff to note in the code you
posted. So here's how _I_ would have written it (based on what you've
posted so far, that is...I don't know enough about the larger design to
comment on how appropriate it really is :) )...
Wherever you had the "initialize DataTable" code before:
DataTable table = new DataTable("System_ID");
using (BackgroundWorker worker = new BackgroundWorker())
{
worker.DoWork += delegate { Global.FillTable(table, "EXEC
AcpReport_GetSystemIDs"); };
worker.RunWorkerCompleted += delegate
{
foreach (DataRow row in table.Rows)
{ // add the generic search first
cboFilter2.Items.Add(row["System_ID"].ToString().Trim());
}
};
worker.RunWorkerAsync();
}
and in your "Global" class:
public static void FillTable(DataTable table, string sqlCommand)
{
SqlDataAdapter da = new SqlDataAdapter(sqlCommand, g_conn);
int records = -1;
try
{
records = da.Fill(table);
}
catch (SqlException err)
{
Console.WriteLine(err.Message);
}
}
Note: I didn't actually compile the above, so my apologies if there's some
typographical error. That's the basic idea though. No need for all the
other awkward sleeping, setting flags, busy-waiting, etc. And yes, it
uses BackgroundWorker. :)
I suppose there's a chance that changing the code to the above would fix
whatever problem you have. But if not, you really do need to post a
concise-but-complete code sample that demonstrates the problem.
Pete
