read() is not thread safe ?

[Zafer Abu saeed]

Hello all,

Unlike delete() and update(), we do not lock a record when we read() it,
so its possible for another thread to interrupt the reading thread, and
delete a record while it is being read.

My implementation of the read() method is:

public String[] read(int recNo) throws RecordNotFoundException {
 
seekRecord(i);
short flag = raf.readShort();
if (flag == DELETED_RECORD) {
throw new RecordNotFoundException("...");
}
//read the record..
....
}

I think the following scenario is probable:

Thread A read the flag and found that the record is not deleted.
Thread B delete the record
Thread A continue reading the record

What do you think?
Thanks.

[mike acre]

Yes that is quite possible, but most of us don't care.
What is important is that when it comes to acting on information provided by a read - ie booking a record or whatever your assignment does during an update, then the checks are made to make sure the record is as expected.

If you just want the latest version of records then you do a new search.
There is no way round this since the results will always be out of date the instant you obtain them.

However, rereading what you put, you should also ensure that all seek followed by read operations are synchronised (ie atomic).

[Andrew Monkhouse]

Hi Zafer,

You actually have a much bigger potential problem in that code snippet. Without any synchronization, the file pointers could move. Assuming that your update method also does not have any synchronization, then the following is possible:

        Thread A  (updating record 5)    Thread B (reading record 7)
        -----------------------------    ---------------------------
        Seeks to record 5
                                         Seeks to record 7
                                         Reads record (pointer now at record 8)
        Writes record (overwriting record 8  :eek:  )

You need to ensure that the file operations are treated as an atomic operation - that is, the seek and the read or write are done together.

One of the things you might want to consider is whether to read or write the entire record, rather than reading or writing individual fields. This will allow you to minimize the amount of synchronized code.

Regards, Andrew
[ October 06, 2004: Message edited by: Andrew Monkhouse ]

[Zafer Abu saeed]

Thanks Mike and Andrew,

I don't know how I didn't think about that, this is an essential idea.

Based on this, I will synchronize the {seek, read} operations,
although this will reduce concurrency dramatically, since some methods -
like update()- will synchronize a large block {seek, read, check cookie, write}.?

[peter wooster]

Originally posted by Zafer Abu saeed:
Thanks Mike and Andrew,

I don't know how I didn't think about that, this is an essential idea.

Based on this, I will synchronize the {seek, read} operations,
although this will reduce concurrency dramatically, since some methods -
like update()- will synchronize a large block {seek, read, check cookie, write}.?

Update doesn't need to do {seek, read, check cookie, write}, it can check the cookie without synch since if its locked the record the cookie won't change. It doesn't need to read before write since the record won't have changed since it locked it. So the synch is only needed on {seek, write}.

So the business operation of booking a record is:
{
lockRecord => {wait(lockMap), checkDeleted => synch(raf){seek,read}}
readRecord => synch(raf){seek,read}
check that record is the same as version in table
updateRecord => synch(raf){seek, write}
unlockRecord => {notifyAll(lockMap)}
}

[Mogens Nidding]

Originally posted by peter wooster:

Update doesn't need to do {seek, read, check cookie, write}, it can check the cookie without synch since if its locked the record the cookie won't change. It doesn't need to read before write since the record won't have changed since it locked it. So the synch is only needed on {seek, write}.

What Peter says about update only needing to sync{seek, write} is correct in the context of a business operation where the record is locked and guaranteed to exist. But in general, the update method does not require a record to be locked in order to update it (in my version of the assignment, anyway). So I think you should do one of the following

not let the update method assume the record is locked, and instead sync the whole {seek, read, check, seek, write} thing

make the update method lock the record while it's operating on it (unless the current client already owns the lock)

make it an explicitly documented precondition of the update method that the record must be locked (not sure I would recommend this since it constrains what can be done with the interface given in the assignment),

Admitted, you do lose a little bit of performance if you do one of the top two things, but in my opinion, it's more important that update follows its general contract, which does not have any prerequisites. A straight forward solution should score higher than a more complicated, but slightly more efficient one.

[Zafer Abu saeed]

Thank you all,

In my assignment, the update() method accepts a cookie, so I think that Peter's thoughts fits my case.

Now as long as you lock() a record before updating it, delete() and update() will not throw a RecordNotFoundException, although this exception is declared to be thrown by these methods. Is this normal ?
(I know its Ok for the compiler, but I'm asking about the grader).

[Zafer Abu saeed]

I'm thinking about this again..

delete() and update() need not to check for deleted records, and will not throw RecordNotFoundException,
but I have in my specifications:

Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file.

So, although we know that there is no need to check inside the update() method (to see if the record is deleted), we have to perform such a check.

Thoughts?
[ October 11, 2004: Message edited by: Zafer Abu saeed ]

[Mogens Nidding]

I have the same considerations: In your case, given that the program is written correctly, if the record does not exist when update is called with a locking, then it will indicate that the caller has supplied an invalid locking cookie.

Normally, we punish callers who pass invalid arguments by throwing an IllegalArgumentException, but in this case, a RecordNotFoundException would also apply, since the interface says it is thrown when the specified record does not exist.

Shortly put, we have two problems, but we can only throw one exception.

So I think there are two solutions (possibly three, since I don't know exactly how your interface looks or whether you check cookie validity):

1) Always throw IllegalArgumentException if the cookie is invalid. A RecordNotFoundException is probably not possible if the cookie is valid. If this is true, there is no need to check it.

2) Always throw RecordNotFoundException if the record is not found. An illegal cookie is possible even when the record exists, so we must check that as a secondary thing.

I would go with the first option for two reasons: a) It is deterministic: If the caller does something wrong, he gets an IllegalArgumentException every time, not different exceptions depending on whether there happens to exist a record with a matching number. b) Even though the record does not exist, the real problem is not the caller tried to lock the wrong record. The real problem is that the caller tried to update a record he did not lock first!

Of course, this is to some extent a matter of taste, so if anyone disagrees, I would like to hear their opinion.
[ October 11, 2004: Message edited by: Nicky Bodentien ]

[Mogens Nidding]

By the way, about the Any methods that throw RecordNotFoundException should do so if a specified record does not exist or is marked as deleted in the database file statement, I think we can be a bit bold and read it as this: When a method throws a RecordNotFoundException, it should be because the specified record does not exist or is marked as deleted, but not necessarily another way around. I think it's a grey zone.

[Rob Shields]

Hi

Sorry to butt in, but it's on a related topic so I thought it's be OK rather than starting a new thread.

Originally posted by peter wooster:
So the business operation of booking a record is:
{
lockRecord => {wait(lockMap), checkDeleted => synch(raf){seek,read}}
readRecord => synch(raf){seek,read}
check that record is the same as version in table
updateRecord => synch(raf){seek, write}
unlockRecord => {notifyAll(lockMap)}
}

lockRecord => synch(this){wait(this), checkDeleted => seek,read}
readRecord => synch(this){seek,read}
check that record is the same as version in table
updateRecord => synch(this){seek, write}
unlockRecord => {notifyAll(this)}

Can anyone see any problems with this approach? It is necessary to wait on a separate object to the one being sync-ed on? I'm concerned I may have missed something.

Regards
Rob Shields
[ October 11, 2004: Message edited by: Rob Shields ]