• Post Reply Bookmark Topic Watch Topic
  • New Topic

Timer task - is it necessary/thread safe?  RSS feed

 
Phil English
Ranch Hand
Posts: 62
MySQL Database Netbeans IDE Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
I am coding up a little test project which puts check in times into a list and then repeatedly checks those times to see if any have expired.

I have coded this up and built a test class and it works just fine for now using a Timer. However, I understand that Timer() creates a new Thread in which to run the TimerTask and have been told (not on here) that multithreading is something to be avoided if possible.

I have added this TimerTask which I invoke every 5 seconds:



So my questions are:
1. When I create the Timer I pass it a reference to CheckList but I believe that there is still only one CheckList object so if I call the add() method on CheckList from the original Thread then the next Timer run will see this new CheckIn object in the CheckList - am I right?
2. What concessions do I need to make to thread safety around the run() method? If the Timer is interrupted during run() and an new checkIn is inserted using add() I think I might get a pointer exception (my coding skills aren't up to testing this though). I thought if I just added changed public void run() to public synchronized void run() that might work?
3. Is this a good approach in general or is there a better way?

I can show all test code if necessary - just didn't want to clog the post.
 
Winston Gutkowski
Bartender
Posts: 10575
66
Eclipse IDE Hibernate Ubuntu
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Phil English wrote:So my questions are:
1. When I create the Timer I pass it a reference to CheckList but I believe that there is still only one CheckList object so if I call the add() method on CheckList from the original Thread then the next Timer run will see this new CheckIn object in the CheckList - am I right?
2. What concessions do I need to make to thread safety around the run() method? If the Timer is interrupted during run() and an new checkIn is inserted using add() I think I might get a pointer exception (my coding skills aren't up to testing this though). I thought if I just added changed public void run() to public synchronized void run() that might work?
3. Is this a good approach in general or is there a better way?

1. If it really is the same Thread, then yes. However, if more than one Thread can call add() then, as usual with multi-threading the answer is: not necessarily, unless checkList itself is synchronized - and the simplest way to do that is with Collections.synchronizedList(), viz:
List<CheckIn> checkList = Collections.synchronizedList(new ArrayList<CheckIn>(10));
Note: that List on the left hand side is important; and actually good practise in general.
Be sure to read the documentation for Collections.synchronizedList(), because it will also affect your 'foreach' loop.

2. See last sentence above. Making run() synchronized will simply stop two Threads from running it at the same time; but since this is a TimerTask and presumably run by some sort of scheduler, I doubt that it's an issue (I could well be wrong though).

3. Well, I don't see anything particularly wrong with the intent; but I've never tried what you're doing. Maybe others have.
A couple of general points though.
  • You only ever seem to add to your 'checkList', which suggests to me that you may end up with a lot of 'expired' CheckIns to plough through. If that's OK with you, then what you have is fine; but you might also want to think about creating a second List of 'expired's and flush them from your 'checkList' when you run your foreach loop.
    Now, if that seems like a good idea, then ArrayList may not be the best type to use: deletes from an ArrayList take O(n) time, whereas deletes from a LinkedList take O(1) time.
  • I don't see anywhere that you use your check() method; but the same strictures that apply to the foreach loop in run() would also apply to it.

  • HIH

    Winston
     
    Phil English
    Ranch Hand
    Posts: 62
    MySQL Database Netbeans IDE Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator

    Thanks for the help!

    Winston Gutkowski wrote:
    1. If it really is the same Thread, then yes. However, if more than one Thread can call add() then, as usual with multi-threading the answer is: not necessarily, unless checkList itself is synchronized - and the simplest way to do that is with Collections.synchronizedList(), viz:
    List<CheckIn> checkList = Collections.synchronizedList(new ArrayList<CheckIn>(10));
    Note: that List on the left hand side is important; and actually good practise in general.
    Be sure to read the documentation for Collections.synchronizedList(), because it will also affect your 'foreach' loop.


    Ok. At present there are two Threads. One which is the Timer Thread the other is the (not sure of the terminology) Original Thread which the rest of the program is running on. Only the run() method is called in the Timer Thread so I think I am safe there. All the administration of the CheckIn list will eventually take place in a GUI in the Original Thread. But I might try that synchronizedList implementation anyway just to see what it does. I can see how it might be better to synchronize the list (as the source of the conflicts) rather than the individual methods which call it so thanks for that thought.

    Re. List<CheckIn> vs. ArrayList<CheckIn>: Thanks for the info. I'll get stuck in to Head First or the internets and work out why that is - I think it will help if I have to change my code later so that it is no longer and ArrayList? But I freely admit that this is one area I'm still getting my head around

    Winston Gutkowski wrote:
    3. Well, I don't see anything particularly wrong with the intent; but I've never tried what you're doing. Maybe others have.
    A couple of general points though.
  • You only ever seem to add to your 'checkList', which suggests to me that you may end up with a lot of 'expired' CheckIns to plough through. If that's OK with you, then what you have is fine; but you might also want to think about creating a second List of 'expired's and flush them from your 'checkList' when you run your foreach loop.
    Now, if that seems like a good idea, then ArrayList may not be the best type to use: deletes from an ArrayList take O(n) time, whereas deletes from a LinkedList take O(1) time.
  • I don't see anywhere that you use your check() method; but the same strictures that apply to the foreach loop in run() would also apply to it.

  • HIH

    Winston


    General points:

    Yes, at the moment I don't have a remove() function because I just built it to the bare minimum to test the Timer implementation but now you mention it I could call that remove() or flush() method from within the Timer Thread which would make a synchronizedList necessary rather than just an exercise in curiosity.

    The check() method is only to be used for 'on demand' rather than scheduled checks on the list and will therefore be run from the Original Thread but I appreciate that regardless of this it will need to be Thread Safe.
     
    Winston Gutkowski
    Bartender
    Posts: 10575
    66
    Eclipse IDE Hibernate Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Phil English wrote:...I could call that remove() or flush() method from within the Timer Thread which would make a synchronizedList necessary rather than just an exercise in curiosity...

    Actually, it's necessary for any update (so add()s as well) and for ensuring that anyone reading 'checkList' sees the latest updates. Don't get the idea that synchronization is just for deletes.

    Winston
     
    Steve Luke
    Bartender
    Posts: 4181
    22
    IntelliJ IDE Java Python
    • Likes 1
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Also, you should understand that GUI related code generally gets run in its own (separate) thread - the Event Dispatcher Thread. so you will have at least three threads to consider in this application: The timer's execution thread, the gui thread, and the main thread (which starts the application).

    If you intend to read/write/manually check the list from the GUI, you definitely need to do synchronization. But make sure you read the documentation that Winston pointed you to, because simply using Collections.synchronizedList() is not enough when you iterate over the list or need to modify the objects in the list.

    Right now, your strategy is to keep a list of all the check in times, then regularly check all of them to see if they are expired. A different approach would be to loop through all the check in times once, and for each one add a timer task to run when it expires. This would probably be more efficient and allow you to work with lots more values. You wouldn't need to iterate the list in the timer task and so it would be easier to use a synchronized list.
     
    Phil English
    Ranch Hand
    Posts: 62
    MySQL Database Netbeans IDE Ubuntu
    • Mark post as helpful
    • send pies
    • Quote
    • Report post to moderator
    Winston Gutkowski wrote:
    Phil English wrote:...I could call that remove() or flush() method from within the Timer Thread which would make a synchronizedList necessary rather than just an exercise in curiosity...

    Actually, it's necessary for any update (so add()s as well) and for ensuring that anyone reading 'checkList' sees the latest updates. Don't get the idea that synchronization is just for deletes.

    Winston


    No worries. I got the idea.

    Steve Luke wrote:Also, you should understand that GUI related code generally gets run in its own (separate) thread - the Event Dispatcher Thread. so you will have at least three threads to consider in this application: The timer's execution thread, the gui thread, and the main thread (which starts the application).

    If you intend to read/write/manually check the list from the GUI, you definitely need to do synchronization. But make sure you read the documentation that Winston pointed you to, because simply using Collections.synchronizedList() is not enough when you iterate over the list or need to modify the objects in the list.

    Right now, your strategy is to keep a list of all the check in times, then regularly check all of them to see if they are expired. A different approach would be to loop through all the check in times once, and for each one add a timer task to run when it expires. This would probably be more efficient and allow you to work with lots more values. You wouldn't need to iterate the list in the timer task and so it would be easier to use a synchronized list.


    Righty ho. I haven't gotten to GUIs yet. I was planning on leaving that to last and getting the underlying business straight first but thanks for the forewarning.

    I really like that design. Each CheckIn object holding/referencing its own Timer. That would limit CheckList to simply holding references to each CheckIn object. Looping through all CheckIn objects once probably won't work because they will come and go as people check in and out but I can write methods into the CheckIn class to handle that. I'll get into that now - thanks very much!

    Another question. Given that, for Timer Tasks the method has to override run() and the return type for run() is void - how do I effectively communicate the expiry/completion of a CheckIn TimerTask? I'm thinking about perhaps an Event with an associated Listener somewhere else is the code but is that necessary or have I missed a trick? I've looked at Javadocs and all the Events seem to be swing package classes - not sure about best practice - seems like misuse of a class? Edit: Urgh. Stupid question. Just need to pass necessary references in constructor of TimerTask.

    Thanks again for all the help.
     
    Don't get me started about those stupid light bulbs.
    • Post Reply Bookmark Topic Watch Topic
    • New Topic
    Boost this thread!