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 DataTableThread er 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 BackgroundWorke r wasn't useful, "daveL"'s suggestion
was actually on-point and relevant :) )...
On Mon, 25 Aug 2008 06:16:01 -0700, jp2msft
<jp*****@discus sions.microsoft .comwrote:
In the main body, the DataTable is constructed with the SQL statement:
DataTable table = new DataTable("Syst em_ID");
Global.FillTabl e(ref table, "EXEC AcpReport_GetSy stemIDs");
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 DataTableThread er. 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(10 0);
Application.DoE vents();
} 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.DoE vents(). 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.DoE vents() 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.Item s.Add(row["System_ID"].ToString().Tri m());
}
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;
DataTableThread er dtParam = new DataTableThread er(ref table,
sqlCommand);
Thread dbThread = new Thread(fillTabl eThread);
dbThread.IsBack ground = true;
dbThread.Priori ty = ThreadPriority. AboveNormal;
Never increase your thread priority. As with Thread.Sleep() and
Application.DoE vents(), it's practically never the right thing to do. And
it's even more dangerous than Application.DoE vents(), 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("Syst em_ID");
using (BackgroundWork er worker = new BackgroundWorke r())
{
worker.DoWork += delegate { Global.FillTabl e(table, "EXEC
AcpReport_GetSy stemIDs"); };
worker.RunWorke rCompleted += delegate
{
foreach (DataRow row in table.Rows)
{ // add the generic search first
cboFilter2.Item s.Add(row["System_ID"].ToString().Tri m());
}
};
worker.RunWorke rAsync();
}
and in your "Global" class:
public static void FillTable(DataT able table, string sqlCommand)
{
SqlDataAdapter da = new SqlDataAdapter( sqlCommand, g_conn);
int records = -1;
try
{
records = da.Fill(table);
}
catch (SqlException err)
{
Console.WriteLi ne(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 BackgroundWorke r. :)
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