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 MainThreadManag ement
{
public delegate string ThreadFinishedC allback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(st ring aFtpSite)
{
ftpSite = aFtpSite;
}
}

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

protected Object GetNextFileLock Obj = new Object(); //locking
object
protected Object NotifyThreadFin ishedLockObj = new Object(); //
locking object

public MainThread(stri ng aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToD ownload()
{
System.Threadin g.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 ThreadFinishedC allback(NotifyT hreadFinished),
sFileName);

aThread = new System.Threadin g.Thread(
new ThreadStart(aCh ildThread.Downl oadFile));

arrChildThreads .Add(aThread);
}

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

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

private string GetNextFileName ()
{
string sFileName;
lock (GetNextFileLoc kObj)
{
if (arrFileNames.C ount 0)
{
sFileName = arrFileNames[0];
arrFileNames.Re moveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPL isting NotifyThreadFin ished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFi nishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName ();
return sFileName;
}
}
}
}

namespace ChildThreadMana gement
{
public class ChildThread : WorkerThread
{
ThreadFinishedC allback NotifyThreadFin ished;
string aFileName;

public ChildThread(Thr eadFinishedCall back aNotifyThreadFi nished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFin ished = aNotifyThreadFi nished;
sFileName = aFileName;
}

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

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

if (bSuccess)
lImageAttribPro p = NotifyThreadFin ished(lIsSucces s,
lImageAttribPro p.sOrderAttribI D);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsCo nnected))
FtpClient.Disco nnect();
FtpClient.Dispo se();
}
}
}

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

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

[Jon Skeet [C# MVP]]

Gina_Marano <gi*******@gmai l.comwrote:

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.

--
Jon Skeet - <sk***@pobox.co m>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[Brian Gideon]

On Feb 8, 12:30 pm, "Gina_Maran o" <ginals...@gmai l.comwrote:

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 MainThreadManag ement
{
public delegate string ThreadFinishedC allback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(st ring aFtpSite)
{
ftpSite = aFtpSite;
}
}

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

protected Object GetNextFileLock Obj = new Object(); //locking
object
protected Object NotifyThreadFin ishedLockObj = new Object(); //
locking object

public MainThread(stri ng aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToD ownload()
{
System.Threadin g.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 ThreadFinishedC allback(NotifyT hreadFinished),
sFileName);

aThread = new System.Threadin g.Thread(
new ThreadStart(aCh ildThread.Downl oadFile));

arrChildThreads .Add(aThread);
}

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

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

private string GetNextFileName ()
{
string sFileName;
lock (GetNextFileLoc kObj)
{
if (arrFileNames.C ount 0)
{
sFileName = arrFileNames[0];
arrFileNames.Re moveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPL isting NotifyThreadFin ished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFi nishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName ();
return sFileName;
}
}
}

}

namespace ChildThreadMana gement
{
public class ChildThread : WorkerThread
{
ThreadFinishedC allback NotifyThreadFin ished;
string aFileName;

public ChildThread(Thr eadFinishedCall back aNotifyThreadFi nished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFin ished = aNotifyThreadFi nished;
sFileName = aFileName;
}

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

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

if (bSuccess)
lImageAttribPro p = NotifyThreadFin ished(lIsSucces s,
lImageAttribPro p.sOrderAttribI D);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsCo nnected))
FtpClient.Disco nnect();
FtpClient.Dispo se();
}
}
}

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

private bool getFtpConnectio n(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]

On Feb 8, 1:38 pm, "Brian Gideon" <briangid...@ya hoo.comwrote:

On Feb 8, 12:30 pm, "Gina_Maran o" <ginals...@gmai l.comwrote:

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 MainThreadManag ement
{
public delegate string ThreadFinishedC allback();

public class WorkerThread
{
protected string ftpSite;

protected WorkerThread(st ring aFtpSite)
{
ftpSite = aFtpSite;
}
}

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

protected Object GetNextFileLock Obj = new Object(); //locking
object
protected Object NotifyThreadFin ishedLockObj = new Object(); //
locking object

public MainThread(stri ng aFTPSite): base(aFTPSite)
{
}

public void ProcessFilesToD ownload()
{
System.Threadin g.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 ThreadFinishedC allback(NotifyT hreadFinished),
sFileName);

aThread = new System.Threadin g.Thread(
new ThreadStart(aCh ildThread.Downl oadFile));

arrChildThreads .Add(aThread);
}

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

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

private string GetNextFileName ()
{
string sFileName;
lock (GetNextFileLoc kObj)
{
if (arrFileNames.C ount 0)
{
sFileName = arrFileNames[0];
arrFileNames.Re moveAt(0);
}
return sFileName;
}
}

//called from child threads using callback
private ChilKatFtp.FTPL isting NotifyThreadFin ished(IsSuccess
aSuccess, string aParent)
{
lock (NotifyThreadFi nishedLockObj)
{
string sFileName = "";
sFileName = GetNextFileName ();
return sFileName;
}
}
}

}

namespace ChildThreadMana gement
{
public class ChildThread : WorkerThread
{
ThreadFinishedC allback NotifyThreadFin ished;
string aFileName;

public ChildThread(Thr eadFinishedCall back aNotifyThreadFi nished,
string aFileName, string aFTPSite)
: base(aFTPSite)
{
NotifyThreadFin ished = aNotifyThreadFi nished;
sFileName = aFileName;
}

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

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

if (bSuccess)
lImageAttribPro p = NotifyThreadFin ished(lIsSucces s,
lImageAttribPro p.sOrderAttribI D);
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsCo nnected))
FtpClient.Disco nnect();
FtpClient.Dispo se();
}
}
}

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

private bool getFtpConnectio n(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 GetNextFileLock Obj locking object. Is
there any thing wrong with NotifyThreadFin ished? 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 = getFtpConnectio n(ftpSite, ref FtpClient);
if (bSuccess)
bSuccess = DoDownloadFile( FtpClient, sFileName);

if (bSuccess)
sFileName = NotifyThreadFin ished(); <-----
else
sFileName = ""; //causes loop to end
} while (sFileName != ""); }
finally {
if (FtpClient != null)
{
if (FtpClient.IsCo nnected))
FtpClient.Disco nnect();
FtpClient.Dispo se();
}
}
}

[Jon Skeet [C# MVP]]

Gina_Marano <gi*******@gmai l.comwrote:

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 GetNextFileLock Obj 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
NotifyThreadFin ishedLockObj myself. So long as GetNextFileName is
thread-safe, it should be fine.

Is there any thing wrong with NotifyThreadFin ished? 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.

--
Jon Skeet - <sk***@pobox.co m>
http://www.pobox.com/~skeet Blog: http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

[kelvin]

On Feb 9, 3:14 pm, Jon Skeet [C# MVP] <[e-mail] wrote:

Gina_Marano <ginals...@gmai l.comwrote:

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 GetNextFileLock Obj locking object.

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

Is there any thing wrong with NotifyThreadFin ished? 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.

--
Jon Skeet - <[e-mail] >http://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

Please send me your more inquiry or requirements to my e-mail address
to ke****@leadingw ay.com.tw

Best regards,

Kelvin Chang

[kelvin]

On Feb 9, 3:14 pm, Jon Skeet [C# MVP] <[e-mail] wrote:

Gina_Marano <ginals...@gmai l.comwrote:

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 GetNextFileLock Obj locking object.

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

Is there any thing wrong with NotifyThreadFin ished? 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.

--
Jon Skeet - <[e-mail] >http://www.pobox.com/~skeet Blog:http://www.msmvps.com/jon.skeet
If replying to the group, please do not mail me too

Please contact me to ke****@leadingw ay.com.tw

Best regards,

Kelvin Chang