multi-threaded app/ using lock

[Gina_Marano]

Hey All,

I am using multiple child threads per main thread to download files.
It sometimes appears as if the same file is being downloaded twice. I
am using "lock". Am I using it correctly? Any blantant threading
errors here?

Any opinions would be greatly appreciated.

namespace MainThreadManagement
{
public delegate string ThreadFinishedCallback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(string aFtpSite)
{
ftpSite = aFtpSite;
}
}

public class MainThread : WorkerThread
{
List<string> arrFileNames = new List<string>();

protected Object GetNextFileLockObj = new Object(); //locking
object
protected Object NotifyThreadFinishedLockObj = new Object(); //
locking object

public MainThread(string aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToDownload()
{
System.Threading.Thread aThread = null;
ChildThread aChildThread;
ArrayList arrChildThreads = new ArrayList();
string sFileName;

//get a list of the files to download
arrFileNames = GetListOfFiles();

//assume maximum of 10 child threads
for (int i = 0; i < 10; i++)
{
sFileName = GetNextFileName();

if (sFileName == "") //no more
break;

aChildThread = new ChildThread(
new ThreadFinishedCallback(NotifyThreadFinished),
sFileName);

aThread = new System.Threading.Thread(
new ThreadStart(aChildThread.DownloadFile));

arrChildThreads.Add(aThread);
}

//lets get all threads ready then start them.
for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Start();
}

for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Join();
}
}

private string GetNextFileName()
{
string sFileName;
lock (GetNextFileLockObj)
{
if (arrFileNames.Count > 0)
{
sFileName = arrFileNames[0];
arrFileNames.RemoveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPListing NotifyThreadFinished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFinishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName();
return sFileName;
}
}
}
}

namespace ChildThreadManagement
{
public class ChildThread : WorkerThread
{
ThreadFinishedCallback NotifyThreadFinished;
string aFileName;

public ChildThread(ThreadFinishedCallback aNotifyThreadFinished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFinished = aNotifyThreadFinished;
sFileName = aFileName;
}

public void DownloadFile()
{
Ftp FtpClient = null;
bool bSuccess;

try {
do {
bSuccess = getFtpConnection(ftpSite, ref FtpClient);
if (bSuccess)
bSuccess = DoDownloadFile(FtpClient, sFileName);

if (bSuccess)
lImageAttribProp = NotifyThreadFinished(lIsSuccess,
lImageAttribProp.sOrderAttribID);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsConnected))
FtpClient.Disconnect();
FtpClient.Dispose();
}
}
}

private bool DoDownloadFile(Ftp FtpClient, string aFileName)
{
//this part works, for simplity of post removed code
return true;
}

private bool getFtpConnection(string aFTPSite, ref Ftp aFtpClient)
{
//this part works, for simplity of post removed code
return true;
}
}
}

 

[Jon Skeet [C# MVP]]

I am using multiple child threads per main thread to download files.
It sometimes appears as if the same file is being downloaded twice. I
am using "lock". Am I using it correctly? Any blantant threading
errors here?

It doesn't look like this is your real code. The constructor for
ChildThread assigns to the variable sFileName, which doesn't appear to
exist as far as I can see. Likewise, your ChildThread constructor takes
2 arguments, but it looks like you're passing in three.

Could you post a short but complete program which demonstrates the
problem?

See http://www.pobox.com/~skeet/csharp/complete.html for details of
what I mean by that.

A few other things to note:

1) Use exceptions for error handling, not boolean return values. It'll
make your code simpler.

2) "break" is a more straightforward way to exit a loop than setting a
variable which is part of the loop condition

The threading itself looks (at first glance) okay, but the code is very
hard to follow.

 

[Brian Gideon]

Hey All,

I am using multiple child threads per main thread to download files.
It sometimes appears as if the same file is being downloaded twice. I
am using "lock". Am I using it correctly? Any blantant threading
errors here?

Any opinions would be greatly appreciated.

namespace MainThreadManagement
{
public delegate string ThreadFinishedCallback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(string aFtpSite)
{
ftpSite = aFtpSite;
}
}

public class MainThread : WorkerThread
{
List<string> arrFileNames = new List<string>();

protected Object GetNextFileLockObj = new Object(); //locking
object
protected Object NotifyThreadFinishedLockObj = new Object(); //
locking object

public MainThread(string aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToDownload()
{
System.Threading.Thread aThread = null;
ChildThread aChildThread;
ArrayList arrChildThreads = new ArrayList();
string sFileName;

//get a list of the files to download
arrFileNames = GetListOfFiles();

//assume maximum of 10 child threads
for (int i = 0; i < 10; i++)
{
sFileName = GetNextFileName();

if (sFileName == "") //no more
break;

aChildThread = new ChildThread(
new ThreadFinishedCallback(NotifyThreadFinished),
sFileName);

aThread = new System.Threading.Thread(
new ThreadStart(aChildThread.DownloadFile));

arrChildThreads.Add(aThread);
}

//lets get all threads ready then start them.
for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Start();
}

for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Join();
}
}

private string GetNextFileName()
{
string sFileName;
lock (GetNextFileLockObj)
{
if (arrFileNames.Count > 0)
{
sFileName = arrFileNames[0];
arrFileNames.RemoveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPListing NotifyThreadFinished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFinishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName();
return sFileName;
}
}
}

}

namespace ChildThreadManagement
{
public class ChildThread : WorkerThread
{
ThreadFinishedCallback NotifyThreadFinished;
string aFileName;

public ChildThread(ThreadFinishedCallback aNotifyThreadFinished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFinished = aNotifyThreadFinished;
sFileName = aFileName;
}

public void DownloadFile()
{
Ftp FtpClient = null;
bool bSuccess;

try {
do {
bSuccess = getFtpConnection(ftpSite, ref FtpClient);
if (bSuccess)
bSuccess = DoDownloadFile(FtpClient, sFileName);

if (bSuccess)
lImageAttribProp = NotifyThreadFinished(lIsSuccess,
lImageAttribProp.sOrderAttribID);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsConnected))
FtpClient.Disconnect();
FtpClient.Dispose();
}
}
}

private bool DoDownloadFile(Ftp FtpClient, string aFileName)
{
//this part works, for simplity of post removed code
return true;
}

private bool getFtpConnection(string aFTPSite, ref Ftp aFtpClient)
{
//this part works, for simplity of post removed code
return true;
}
}



}- Hide quoted text -

- Show quoted text -

Hi,

The DownloadFile method will loop until there is an error downloading
the file. I'm surprised the problem isn't more wide spread. I'm not
seeing the need for two different lock objects. The code is difficult
to follow. And I agree that it can't possibly be what you really
have.

Brian

 

[Gina_Marano]

Hey All,

I am using multiple child threads per main thread to download files.
It sometimes appears as if the same file is being downloaded twice. I
am using "lock". Am I using it correctly? Any blantant threading
errors here?

Any opinions would be greatly appreciated.

namespace MainThreadManagement
{
public delegate string ThreadFinishedCallback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(string aFtpSite)
{
ftpSite = aFtpSite;
}
}

public class MainThread : WorkerThread
{
List<string> arrFileNames = new List<string>();

protected Object GetNextFileLockObj = new Object(); //locking
object
protected Object NotifyThreadFinishedLockObj = new Object(); //
locking object

public MainThread(string aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToDownload()
{
System.Threading.Thread aThread = null;
ChildThread aChildThread;
ArrayList arrChildThreads = new ArrayList();
string sFileName;

//get a list of the files to download
arrFileNames = GetListOfFiles();

//assume maximum of 10 child threads
for (int i = 0; i < 10; i++)
{
sFileName = GetNextFileName();

if (sFileName == "") //no more
break;

aChildThread = new ChildThread(
new ThreadFinishedCallback(NotifyThreadFinished),
sFileName);

aThread = new System.Threading.Thread(
new ThreadStart(aChildThread.DownloadFile));

arrChildThreads.Add(aThread);
}

//lets get all threads ready then start them.
for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Start();
}

for (int j = 0; j < arrChildThreads.Count; j++)
{
aThread = (System.Threading.Thread)arrChildThreads[j];
aThread.Join();
}
}

private string GetNextFileName()
{
string sFileName;
lock (GetNextFileLockObj)
{
if (arrFileNames.Count > 0)
{
sFileName = arrFileNames[0];
arrFileNames.RemoveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPListing NotifyThreadFinished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFinishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName();
return sFileName;
}
}
}

namespace ChildThreadManagement
{
public class ChildThread : WorkerThread
{
ThreadFinishedCallback NotifyThreadFinished;
string aFileName;

public ChildThread(ThreadFinishedCallback aNotifyThreadFinished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFinished = aNotifyThreadFinished;
sFileName = aFileName;
}

public void DownloadFile()
{
Ftp FtpClient = null;
bool bSuccess;

try {
do {
bSuccess = getFtpConnection(ftpSite, ref FtpClient);
if (bSuccess)
bSuccess = DoDownloadFile(FtpClient, sFileName);

if (bSuccess)
lImageAttribProp = NotifyThreadFinished(lIsSuccess,
lImageAttribProp.sOrderAttribID);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsConnected))
FtpClient.Disconnect();
FtpClient.Dispose();
}
}
}

private bool DoDownloadFile(Ftp FtpClient, string aFileName)
{
//this part works, for simplity of post removed code
return true;
}

private bool getFtpConnection(string aFTPSite, ref Ftp aFtpClient)
{
//this part works, for simplity of post removed code
return true;
}
}

}- Hide quoted text -

- Show quoted text -

Hi,

The DownloadFile method will loop until there is an error downloading
the file. I'm surprised the problem isn't more wide spread. I'm not
seeing the need for two different lock objects. The code is difficult
to follow. And I agree that it can't possibly be what you really
have.

Brian- Hide quoted text -

- Show quoted text -

Sorry this isn't the complete program, the program is 200+ lines of
code implementing the FTP and all. I tried to clean it up and dumb
down for posting purposes. I am more interested in the locking logic.

You're right, I don't need the GetNextFileLockObj locking object. Is
there any thing wrong with NotifyThreadFinished? Am I correctly
locking it? Is everything declared correctly so when I think I am
deleting the file name from the list, I actually am? Notice that the
child thread is in its own namespace (actually a different unit). I
don't know if this is causing a problem. I am using a callback to the
main thread to get the next file name.

It is hard to reproduce since it is a multi-threaded problem. But
sometimes is appears that 2 threads are trying to download the same
file.

DownloadFile should read:

public void DownloadFile()
{
Ftp FtpClient = null;
bool bSuccess;

try {
do {
bSuccess = getFtpConnection(ftpSite, ref FtpClient);
if (bSuccess)
bSuccess = DoDownloadFile(FtpClient, sFileName);

if (bSuccess)
sFileName = NotifyThreadFinished(); <-----
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsConnected))
FtpClient.Disconnect();
FtpClient.Dispose();
}
}
}

 

[Jon Skeet [C# MVP]]

Sorry this isn't the complete program, the program is 200+ lines of
code implementing the FTP and all. I tried to clean it up and dumb
down for posting purposes. I am more interested in the locking logic.

The trick, however, is to still post a complete program.

You're right, I don't need the GetNextFileLockObj locking object.

Well, you need to lock on *something* when fetching the next file - but
you only need one lock, really. I dno't see any need for
NotifyThreadFinishedLockObj myself. So long as GetNextFileName is
thread-safe, it should be fine.

Is there any thing wrong with NotifyThreadFinished? Am I correctly
locking it? Is everything declared correctly so when I think I am
deleting the file name from the list, I actually am? Notice that the
child thread is in its own namespace (actually a different unit). I
don't know if this is causing a problem. I am using a callback to the
main thread to get the next file name.

That shouldn't be the problem.

It is hard to reproduce since it is a multi-threaded problem. But
sometimes is appears that 2 threads are trying to download the same
file.

If it's hard to reproduce, that's all the more reason to try to produce
a short but complete program that demonstrates the problem - even if it
only demonstrates it sometimes.

 

[kelvin]

The trick, however, is to still post a complete program.


Well, you need tolockon *something* when fetching the next file - but
you only need onelock, really. I dno't see any need for
NotifyThreadFinishedLockObj myself. So long as GetNextFileName is
thread-safe, it should be fine.


That shouldn't be the problem.


If it's hard to reproduce, that's all the more reason to try to produce
a short but complete program that demonstrates the problem - even if it
only demonstrates it sometimes.

Please send me your more inquiry or requirements to my e-mail address
to (e-mail address removed)

Best regards,

Kelvin Chang

 

[kelvin]

The trick, however, is to still post a complete program.


Well, you need tolockon *something* when fetching the next file - but
you only need onelock, really. I dno't see any need for
NotifyThreadFinishedLockObj myself. So long as GetNextFileName is
thread-safe, it should be fine.


That shouldn't be the problem.


If it's hard to reproduce, that's all the more reason to try to produce
a short but complete program that demonstrates the problem - even if it
only demonstrates it sometimes.

Please contact me to (e-mail address removed)

Best regards,

Kelvin Chang