Synchronized method keeps getting called

I'm still somewhat new to multithreading so mayb开发者_C百科e this is an easy answer, but I tried searching around with locks and synchronized methods but this isn't working out right.

I have a system which connects to an ftp site and downloads an xml file to process. The application waits until the file on the ftp site is updated, if it isn't updated it doesn't do anything. If it is updated it downloads the xml file to start processing.

Before we process the xml we check the version of a file on the server, and the version of something in the database, if the versions aren't the same, create a new thread and update the file on the server to the new version.

So in code it's pretty much:

if(file.version.equals(db.version))
{ 
  do work...
}
else 
{
  startUpdateThread();
}

private synchronized void startUpdateThread()
{
   Runnable updateRunnable = new UpdateRunnable();
   Thread updateThread = new Thread(updateRunnable);
   updateThread.start();
}

The problem I'm having is that the xml file will change on the ftp site in the middle of updating the file on the server and still enter the startUpdateThread() method and by the time it's finished I will have multiple threads trying to accomplish the same tasks.

My question I guess is, why is the syncrhonized method still being called even though it should be locked, and if I'm doing it wrong what is the correct way to approach this without creating multiple threads to accomplish the same tasks.

edit A little further explanation of the system, the file on the server gets loaded into a cache when the application starts, so the file.version.equals is really from a cache. The file only needs to be regenerated once every few months it's something that is done manually that I am going to make automated.

The file on the server and the xml file have nothing to do with each other, it just happens that this check is triggered when the application see's the file on the server has changed (which is roughly every two minutes). To generate a new file and load it into cache takes about 10 minutes. So the check happens about 5 times, thus creating 5 threads, regenerating a file.

The desired result is:

Normal processing to continue with the xml file

ONE new thread being created to reload a totally seperate file on the server.

Hopefully that makes better sense...

Of course - the method startUpdateThread() is syncronized but not the actual execution of the threads. The lock is released when the updateThread.start() call terminates - and this is long before the worker thread finishes.

Question is, what is your desired behaviour: if the remote file changes while you process it, then it is my understanding, that the changed file has a different number.

You could send a signal to the running worker thread to stop processing that file because it is already outdated. Or do you want the "second" worker thread to wait until the first one has finished? In that case, a ThreadPool with size 1 could help.

I think you have a misunderstanding about the concurrency constructs in Java. startUpdateThread() will start the update thread and return instantly, hence the lock is useless. What you probably need is to store updateThread as a field, and check if it is null during your synchronized (this) lock before calling startUpdateThread.

I suggest you have one task to do the checking, and one or more tasks for the processing/download.

The problem you have is that many threads are performing the check at once and they all see the file needs to be downloaded, so they all call startUpdateThread()

The simple way around this is to synchornize the check as well so that only one thread can perform the check as see that it need to be updated.

However, since you are start a new thread everytime, its not clear to me why you are calling this method in a multi-threade dmanner at all. I would just use one thread for the checking.

Obviously, you shouldn't check a file again while its being update. ;)

Rather than starting a Thread everytime, I suggest you use an ExecutorService to pool your threads.

I got a bit lost in the logic of what you're doing, but I can tell you how 'synchronized' works and then you can make sure your logic follows that.

When you make a "normal" instance method (like this one) synchronized, then this effectively means that only one thread at a time can be executing any synchronized method on the same object. (Or can be inside any code explicitly synchronizing on the objcet in question.)

I don't know if this affects what you're doing, but notice that I could have a class as follows:

public class MyClass {
  public synchronized void someMethod() {
    ...
  }
}

and then still have two separate instances of MyClass, each with a call to someMethod() running simultaneously in different threads. On the other hand, on the same instance of MyClass, I couldn't have two calls to someMethod() running simultaneously.

If you're getting a bit lost with synchronized, then you could also consider using one of the explicit Locks introduced in Java 5. (In case it helps, I have some articles on Java 5 Locks and indeed on synchronization and concurrency in general on my web site that you might want to take a look at.)