HashMap and ArrayList adding while iterating/looping

Question

I have a game where every X seconds it will write changed values in memory back to my DB. These values are stored in containers(HashMaps and ArrayLists) when the data they hold is edited.

For simplicity lets pretend I have only 1 container to write to the DB:

public static HashMap<String, String> dbEntitiesDeletesBacklog = new HashMap<String, String>();

My DB writing loop:

Timer dbUpdateJob = new Timer();
dbUpdateJob.schedule(new TimerTask() {
    public void run() {
        long startTime = System.nanoTime();
        boolean updateEntitiesTableSuccess = UpdateEntitiesTable();
        if (!updateEntitiesTableSuccess){
            try {
                conn.rollback();
            } catch (SQLException e) {
                e.printStackTrace();
                logger.fatal(e.getMessage());
                System.exit(1);
            }
        } else { //everything saved to DB - commit time
            try {
                conn.commit();
            } catch (SQLException e) {
                e.printStackTrace();
                logger.fatal(e.getMessage());
                System.exit(1);
            }
        }
        logger.debug("Time to save to DB: " + (System.nanoTime() - startTime) / 1000000 + " milliseconds");
    }
}, 0, 10000); //TODO:: figure out the perfect saving delay

My update method:

private boolean UpdateEntitiesTable() {
    Iterator<Entry<String, String>> it = dbEntitiesDeletesBacklog.entrySet().iterator();
    while (it.hasNext()) {
        Entry<String, String> pairs = it.next();
        String tmpEntityId = pairs.getKey();

        int deletedSuccess = UPDATE("DELETE" + 
                " FROM " + DB_NAME + ".entities" + 
                " WHERE entity_id=(?)", new String[]{tmpEntityId});
        if (deletedSuccess != 1) {
            logger.error("Entity " + tmpEntityId + " was unable to be deleted.");
            return false;
        }
        it.remove();
        dbEntitiesDeletesBacklog.remove(tmpEntityId);
    }

Do I need to create some sort of locking mechanism while 'saving to DB' for the dbEntitiesDeletesBacklog HashMap and other containers not included in this excerpt? I would think I need to, because it creates its iterator, then loops. What if something is added after the iterator is created, and before its done looping through the entries. I'm sorry this is more of a process question and less of a code help question(since I included so much sample code), but I wanted to make sure it was easy to understand what I am trying to do and asking.

Same question for my other containers which I use like so:

public static ArrayList<String> dbCharacterDeletesBacklog =  new ArrayList<String>();

private boolean DeleteCharactersFromDB() {
    for (String deleteWho : dbCharacterDeletesBacklog){
        int deleteSuccess = MyDBSyncher.UPDATE("DELETE FROM " + DB_NAME + ".characters" +
                " WHERE name=(?)", 
                new String[]{deleteWho});

        if (deleteSuccess != 1) {
            logger.error("Character(deleteSuccess): " + deleteSuccess);
            return false;
        }
    }
    dbCharacterDeletesBacklog.clear();
    return true;
}

Thanks so much, as always, for any help on this. It is greatly appreciated!!

Sounds like all you need to do is synchronize the methods accessing your HashMap and ArrayList. — Joel
– Joel, Commented Aug 12, 2013 at 20:44
Yes I was going to say blocking would not be acceptable to me. The game needs to run smooth, and the DB updates are lower priority. I am thinking it may be best for me to lock on(dbCharacterDeletesBacklog) copy (dbCharacterDeletesBacklog) to dest(dbCharacterDeletesBacklog_copy) and then clear (dbCharacterDeletesBacklog). — KisnardOnline
– KisnardOnline, Commented Aug 12, 2013 at 21:07
@JayAvon: Simple synchronization on the collections will certainly work and may even be a good workable solution if you expect there to be only minimal contention for your collection by all the threads that may be running in your application. If you believe that there could be periods of high contention, then you would very likely find that using ConcurrentHashMap would be much more performant. The only way to know is to run your app under actual conditions ... and profile it. — scottb
– scottb, Commented Aug 15, 2013 at 20:06

Trevor Freeman · Accepted Answer · 2013-08-15 20:41:57Z

2

At the very least, you need a synchronized map (via Collections.synchronizedMap) if you are accessing your map concurrently, otherwise you may experience non deterministic behaviour.

Further than that, as you suggest, you also need to lock your map during iteration. From the javadoc for Collections.synchronizedMap() the suggestion is:

It is imperative that the user manually synchronize on the returned map when iterating over any of its collection views:

Map m = Collections.synchronizedMap(new HashMap());
      ...
Set s = m.keySet();  // Needn't be in synchronized block
      ...
synchronized(m) {  // Synchronizing on m, not s!
    Iterator i = s.iterator(); // Must be in synchronized block
    while (i.hasNext())
        foo(i.next());
}

Failure to follow this advice may result in non-deterministic behavior.

Alternatively, use a ConcurrentHashMap instead of a regular HashMap to avoid requiring synchronization during iteration. For a game, this is likely a better option since you avoid locking your collection for a long period of time.

Possibly even better, consider rotating through new collections such that every time you update the database you grab the collection and replace it with a new empty one where all new updates are written to, avoiding locking the collection while the database writes are occurring. The collections in this case would be managed by some container to allow this grab and replace to be thread safe. <<< Note: You cannot expose the underlying collection in this case to modifying code since you need to keep its reference strictly private for the swap to be effective (and not introduce any race conditions).

edited Aug 15, 2013 at 20:41

answered Aug 12, 2013 at 20:57

Trevor Freeman

7,2722 gold badges24 silver badges41 bronze badges

Sign up to request clarification or add additional context in comments.

11 Comments

KisnardOnline Over a year ago

My DB update part of my code is lower priority than other threads so I dont want to block for it. Would something like this work: I am thinking it may be best for me to lock on(dbCharacterDeletesBacklog) copy (dbCharacterDeletesBacklog) to (dbCharacterDeletesBacklog_copy), clear (dbCharacterDeletesBacklog), and then unlock. Then I can update DB with (dbCharacterDeletesBacklog_copy).

Trevor Freeman Over a year ago

@JayAvon That would work, and is essentially similar to using a new collection for additional update writes (just slightly more expensive I imagine due to the copying). Just make sure that your collection is either a synchronizedMap or you are ensuring that you are locking on it manually when adding / updating it in your other threads.

KisnardOnline Over a year ago

Thanks so much for all of the help. I really appreciate it.

KisnardOnline Over a year ago

I hate to even ask with how much you helped me already, but I am having trouble coming up with a non cludgy concept of 'rotating through new collections', by either copying or have lots of if statements. Do you have any thoughts on this?

Trevor Freeman Over a year ago

@JayAvon That depends on if you need to use the full map interface when dealing with your collections. Easiest case is that you do not, so you can have a custom class that encapsulates your data and only has put and get methods which work on an underlying map (which is only exposed when replaced). If that is not possible, then you can extend Guava's ForwardingMap to create an encapsulated map that can be switched out. This is somewhat complicated so probably best to start with your proposed solution (clear, with appropriate locking!), and optimize if it is too slow or incurs too much gc.

|

KisnardOnline · Accepted Answer · 2013-08-15 19:54:10Z

0

Here is a sample of what I will be using. I am posting there here in the hopes that it will help someone else with a similar issue.

public class MyDBSyncher {

    public static boolean running = false;
    public static HashMap<String, String> dbEntitiesInsertsBacklog_A = new HashMap<String, String>();
    public static HashMap<String, String> dbEntitiesInsertsBacklog_B = new HashMap<String, String>();

    public MyDBSyncher(){
        Timer dbUpdateJob = new Timer();
        dbUpdateJob.schedule(new TimerTask() {
            public void run() {
                running = true;
                boolean updateEntitiesTableSuccess = UpdateEntitiesTable();
                running = false;
            }
        }, 0, 10000); //TODO:: figure out the perfect saving delay
    }

    public HashMap getInsertableEntitiesHashMap(){
        if (running){
            return dbEntitiesInsertsBacklog_B;
        } else {
            return dbEntitiesInsertsBacklog_A;
        }
    }

    private boolean UpdateEntitiesTable() {
        Iterator<Entry<String, String>> it2 = getInsertableEntitiesHashMap().entrySet().iterator();
        while (it2.hasNext()) {
            Entry<String, String> pairs = it2.next();
            String tmpEntityId = pairs.getKey();

            //some DB updates here

            it2.remove();
            getInsertableEntitiesHashMap().remove(tmpEntityId);
        }
        return true;
    }
}

answered Aug 15, 2013 at 19:54

KisnardOnline

7604 gold badges17 silver badges43 bronze badges

4 Comments

scottb Over a year ago

If it works, that is good. A problem is that bugs arising from race conditions or other (non-deterministic) concurrency issues can be hard to evoke and may only turn up much later, particularly if your app transitions to a higher concurrency load with more contention for resources. One thing that definitely bothers me is the running variable. As it stands in this solution, the Java Memory Model makes NO GUARANTEES AT ALL that updates to this variable will be visible to other threads unless access is synchronized. If your needs are simple, a volatile declaration may also suffice.

Trevor Freeman Over a year ago

if I am understanding the code correctly, then I think it may have some problems. Primarily when this class is iterating concurrent access will add items to your _A map which is never iterated over. Secondly, as scottb says, you need ensure synchronization of at least the running variable (for which volatile should be enough).

KisnardOnline Over a year ago

yeah good catch, I was thinking the solution is to iterate and copy A->B after B is done writing to the db, but then I have to lock again... so annoying, I'll keep brainstorming... the game is a mmorpg I have been programming for over 2 years now. That is my biggest fear, that when I have users play, hopefully in large numbers, that I get so many concurrent issues.

scottb Over a year ago

@JayAvon: If you want to write a system that you anticipate will be (or want to grow into) a highly concurrent system, then you really do need to read Java Concurrency in Practice, by Brian Goetz and other Java development notables. Joshua Bloch is a contributor and some of the book sections ring comfortably familiar. This book is an indispensable encapsulation (pun) of a number of best practices for concurrent programming in Java. The investment in reading it will pay off in big dividends.

Collectives™ on Stack Overflow

HashMap and ArrayList adding while iterating/looping

2 Answers 2

11 Comments

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

11 Comments

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related