Synchronization in the SCJD Exam with J2SE book

[Sean Keane]

Hi guys, I'm trying to get my head around the synchronization in the SCJD Exam with J2SE book by Monkhouse & Camerlengo. Would anyone be able to tell me if my understanding is correct here?

I'm trying to understand the persistDvd method from the DvdFileAccess class. I've copied the method below.

The DvdFileAccess class does all the access and manipulation of the physical file that represents the database. The persistDvd method writes records to the database.

Would I be correct in thinking how the persistDvd is implemented could cause problems and lead to the file being corrupted when multiple threads are involved?

Take the scenario of adding a record. Assume we have two threads that are both executing the persistDvd method. We'll call these threads Thread-1 and Thread-2. So...

1.0) Thread-1 is trying to add a record that does not exist and gets an offset of null.
2.0) Then Thread-2 is trying to add a record that does not exist and gets an offset of null
3.0) Then Thread-1 goes to add its record, at offset=database.length.
4.0) Then Thread-2 goes to add its record, at offset=database.length.
........4.1) database.length hasn't changed since Thread-1 looked at it!
........4.2) the write lock is only around getting the offset and adding the record to the cache\Map.
........4.3) so both threads could potentially have the same offset?


If this scenario is correct, then depending on which thread gets the lock on database first, it will have the value it wrote to the database overwritten by the thread the comes after it.

class DvdFileAccess {

    /**
     * The physical file on disk containing our Dvd records.
     */
    private RandomAccessFile database = null;

    /**
     * Contains an index for our primary key (UPC number) to file location.
     */
    private static Map<String, Long> recordNumbers
            = new HashMap<String, Long>();

    /**
     * Ensures that many users can read the <code>recordsNumbers</code>
     * collection as long as nobody is updating it.
     */
    private static ReadWriteLock recordNumbersLock
            = new ReentrantReadWriteLock();

    /**
     * This method persists a Dvd object to the database. If the Dvd does not
     * currently exist in the database( as determined by it's unique upc code).
     * then the Dvd will be created.
     *
     * @param dvd The Dvd to store
     * @param create true for creation, false for modification
     * @return Indicates success or failure
     * @throws IOException Indicates there is a problem accessing the data file
     */
    private boolean persistDvd(DVD dvd, boolean create) throws IOException {
        log.entering("DvdFileAccess", "persistDvd", dvd);

        // Perform as many operations as we can outside of the synchronized
        // block to improve concurrent operations.

        Long offset = 0L;
        recordNumbersLock.readLock().lock();      
        try {
            offset = recordNumbers.get(dvd.getUPC());
        } finally {
            recordNumbersLock.readLock().unlock();
        }

        if (create == true && offset == null) {
            recordNumbersLock.writeLock().lock();
            try {
                offset = database.length();
                recordNumbers.put(dvd.getUPC(), offset);
            } finally {
                recordNumbersLock.writeLock().unlock();
            }
            log.info("creating record " + dvd.getUPC());
        } else if (create == false && offset != null) {
            log.info("updating existing record " + dvd.getUPC());
        } else {
            return false;
        }

        final StringBuilder out = new StringBuilder(emptyRecordString);

        /** assists in converting Strings to a byte[] */
        class RecordFieldWriter {
            /** current position in byte[] */
            private int currentPosition = 0;
            /**
             * converts a String of specified length to byte[]
             *
             * @param data the String to be converted into part of the byte[].
             * @param length the maximum size of the String
             */
            void write(String data, int length) {
                out.replace(currentPosition,
                            currentPosition + data.length(),
                            data);
                currentPosition += length;
            }
        }
        RecordFieldWriter writeRecord = new RecordFieldWriter();

        writeRecord.write(dvd.getUPC(), DVD.UPC_LENGTH);
        writeRecord.write(dvd.getName(), DVD.NAME_LENGTH);
        writeRecord.write(dvd.getComposer(), DVD.COMPOSER_LENGTH);
        writeRecord.write(dvd.getDirector(), DVD.DIRECTOR_LENGTH);
        writeRecord.write(dvd.getLeadActor(), DVD.LEAD_ACTOR_LENGTH);
        writeRecord.write(dvd.getSupportingActor(),
                          DVD.SUPPORTING_ACTOR_LENGTH);
        writeRecord.write(dvd.getYear(), DVD.YEAR_LENGTH);
        writeRecord.write("" + dvd.getCopy(), DVD.COPIES_LENGTH);

        // now that we have everything ready to go, we can go into our
        // synchronized block & perform our operations as quickly as possible
        // ensuring that we block other users for as little time as possible.

        synchronized (database) {
            database.seek(offset);
            database.write(out.toString().getBytes());
        }

        log.exiting("DvdFileAccess", "persistDvd", true);
        return true;
    }
}    

[Sean Keane]

I did a little searching, and no surpise, this has come up before

One particular thread is here https://coderanch.com/t/188453/java-developer-SCJD/certification/Synchronization-locking-persistDVD-Andrew-book

So I would guess the approach taken in the book would not be the recommended approach for the assignment, as it is not thread safe?

It sounds like most people synchronize all the methods of the data class, which should avoid any thread safety issues such as with those in implemention in the book?

[Sean Keane]

Some other good posts here:

https://coderanch.com/t/483465/java-developer-SCJD/certification/synchronized-ReentrantReadWriteLock

https://coderanch.com/t/189944/java-developer-SCJD/certification/Andrew-book-persistDvd-method-ERROR#925537

https://coderanch.com/t/188279/java-developer-SCJD/certification/Monkhouse-book-Threading-persistDVD-method#916967

An FAQ with these common types of questions & answers documented would be good. Is there one anywhere?

[Sean Keane]

Andrew actually has an update in his errata for this particular problem

349                 offset = database.length();
349a                while (recordNumbers.containsValue(offset)) {
349b                    offset += DVD.RECORD_LENGTH;
349c                }
350                 recordNumbers.put(dvd.getUPC(), offset);

It seems a little non intuitive though with the while loop. Since each record is a fixed length, and records are added one after another, then it seems to be me that we simply want to get the max value for the offsets in the map. Based on this, does it make sense to be searching the entire set of values possibly multiple times?

I would think a more intuitive solution to this would be:

offset = database.length();
if (recordNumbers.containsValue(offset)) {
   offset = Collections.max(recordNumbers.values()) + DVD.RECORD_LENGTH;
}
recordNumbers.put(dvd.getUPC(), offset);

Or maybe even this:

maxOffset =  Collections.max(recordNumbers.values());
offset = (offset <= maxOffset ? maxOffset + DVD.RECORD_LENGTH : offset);
recordNumbers.put(dvd.getUPC(), offset);

Or maybe you could just set your cache up to always store the maxOffset value, that way there would be no searching required at all?

[Roel De Nijs]

I almost forgot this thread

Clearly your doubts are correct: you can get in trouble because offset calculation is not thread-safe.

It sounds like most people synchronize all the methods of the data class, which should avoid any thread safety issues such as with those in implemention in the book?

I don't think that's completely true. Also a bunch of people work with synchronized blocks or with the new concurrency api. It makes no difference which approach you choose, just implement a thread-safe Data class which meets the requirements.

Or maybe you could just set your cache up to always store the maxOffset value, that way there would be no searching required at all?

Maybe the approach provided is not the most intuitive one and your proposals are fine, but don't forget you won't get more credit because your code calculates the correct offset in 3 lines (and other ones need 5). You may even lose points, because your code might be hard to read and/or maintain. Maybe in your implementation storing the maxOffset value makes sense, but in my implementation it does not (because I reuse deleted entries). So that's completely your choice.

An FAQ with these common types of questions & answers documented would be good. Is there one anywhere?

There is only a FAQ with general (sometimes outdated) questions about this certification. No FAQ about general questions on the Monkhouse book, because honestly it was a long time we had questions about the book in particular. Most questions are concepts from the book applied to the specific situation/code of a rancher. Before Roberto and I took over this forum, Andrew was the moderator here and I guess he got a lot more questions about his own book (which makes more sense, because we don't have any clue what was happening in his thoughts when he was writing these lines of code )

Hope it helps!
Kind regards,
Roel

[Sean Keane]

Cheers for the reply Roel.

That clears up my concerns about the synchronization approach taken in this class. It sounds like with the changes from the errata, that this class should work without threading issues.

The thought on the FAQ was not to have one for the topics in the book, but to have an FAQ for the general areas that people need to consider when designing and implementing their assignment.

I don't mean a list of the areas with complete solutions for the reader , but maybe the main areas that require consideration listed along with various approaches or various threads that discuss an individual area.

This is in fact what I am doing myself in my own notes, gathering various references to threads under a particular topic, to review peoples approachs, problems they encountered, various solutions etc.

[Roel De Nijs]

That's why this forum (just like any other) has an excellent search engine

[Sean Keane]

Yep, I use the search engine, or Google (query site:coderanch.com). A lot of sites that have a search engine also have sticky threads for commonly asked questions and commonly discussed topics. Just a thought

[Roel De Nijs]

True!

But you can't compare a forum like this one with another forum on the internet. The main purpose of this forum is to help somebody with a question/doubt about his/her assignment. It's not the intention to make everybody submit a solution with a singleton Data class with all synchronized methods and a record cache using wait/notifyAll-mechanism.
With Oracle's take-over there was plenty of questions about the submission process, so we started a sticky thread to summarize the current flow of the certification and group all valuable information together. But now the process is better known, so the stickyness will be removed by the end of the month.