All methods synchronized in Data class

[Paweł Baczyński]

Hello everyone!
I've read on this forum that many of you marked all methods in Data class as synchronized.
I'm tempted with this idea because it would allow me to save much time that would be spent on writing proper synchronization-machanisms.
Also, I would not worry so much about thread-safety.
But how to justify this?
How evaluators see taking this "easy just-synchronize-all-stuff way"?
Why should one find method wait for another find to complete etc?

[Himai Minh]

In my assignment, I used ReentrantLock for all the methods that implements create, delete, update and read.
It seems to me that I have to do that. It is because no two threads should access to the DB file at the same time.

[Paweł Baczyński]

Yeah, I didn't mention I decided to keep my database in cache Map<Long, String[]> so really two threads can read it at the same time.
That is why I'm concerned about synchronizing all methods...

[Himai Minh]

Do you use this approach:
1. update the cache when a record is created/deleted/updated
2. save the cache to the db file
?

If that is the case, make sure no two threads save any data to the same db file.
And also, make sure only one thread deletes a record at a time. If one thread iterates the Map and another thread deletes a record in the Map, that may generate concurrent access error.

[Henry Wong]

Pawel Pawlowicz wrote:
I've read on this forum that many of you marked all methods in Data class as synchronized.
I'm tempted with this idea because it would allow me to save much time that would be spent on writing proper synchronization-machanisms.
Also, I would not worry so much about thread-safety.
But how to justify this?
How evaluators see taking this "easy just-synchronize-all-stuff way"?
Why should one find method wait for another find to complete etc?

Keep in mind that "synchronized all methods" does *NOT* guarantee thread-safety. For example, all the methods of the Vector class are synchronized -- but if you need to have a method to enumerate through an unchanging vector (meaning no thread can change it until completion), it will not work. The vector class can't make such a guarantee, and hence, can not be considered thread-safe for the method.

Henry

[Roel De Nijs]

Marking all methods as synchronized is just a part of the solution (like Henry already mentioned).

But if you are looking for justification, it's clearly stated in the instructions of the assignment: A clear design, such as will be readily understood by junior programmers, will be preferred to a complex one, even if the complex one is a little more efficient.

[Paweł Baczyński]

Henry Wong wrote:Keep in mind that "synchronized all methods" does *NOT* guarantee thread-safety.

You are right .
But all methods in Data class are meant to be atomic. They are (in my case): read, update, delete, find, create, lock, unlock.
The only use-case that could not be thread-safe is:
1. findByCriteria
2. read (all found records)
Between step 1 and 2(or during 2) something might changed found records making them invalid.
But this can be easilly solved by checking again resulting set against criteria.

My only cocern was I could lost some points by taking "the easiest way".

[Roel De Nijs]

Pawel Pawlowicz wrote:But all methods in Data class are meant to be atomic. They are (in my case): read, update, delete, find, create, lock, unlock.

It also depends on the number of instances of Data class

And I can think of other use cases which will not be thread-safe, e.g.
- ThreadA reads record 1 and sees it's available
- ThreadB reads record 1 and sees it's available
- ThreadA locks, updates and unlocks record 1
- ThreadB locks, updates and unlocks record 1

The reservation of ThreadA is overwritten by ThreadB and you have a double booking of room/contractor.

[Roel De Nijs]

Pawel Pawlowicz wrote:My only cocern was I could lost some poiint by taking "the easiest way".

I took the "easy road" and scored full marks, so your concern is unnecessary

[Paweł Baczyński]

Roel De Nijs wrote:

Pawel Pawlowicz wrote:But all methods in Data class are meant to be atomic. They are (in my case): read, update, delete, find, create, lock, unlock.

It also depends on the number of instances of Data class

My instructions say: You may assume that at any moment, at most one program is accessing the database file; therefore your locking system only needs to be concerned with multiple concurrent clients of your server.
So is the number of instances a problem?

Roel De Nijs wrote:
And I can think of other use cases which will not be thread-safe, e.g.
- ThreadA reads record 1 and sees it's available
- ThreadB reads record 1 and sees it's available
- ThreadA locks, updates and unlocks record 1
- ThreadB locks, updates and unlocks record 1

The reservation of ThreadA is overwritten by ThreadB and you have a double booking of room/contractor.

You are right. But I think this is a problem for a business layer, not the Data class.

[Roel De Nijs]

Pawel Pawlowicz wrote:
My instructions say: You may assume that at any moment, at most one program is accessing the database file; therefore your locking system only needs to be concerned with multiple concurrent clients of your server.
So is the number of instances a problem?

One program (application) is accessing the database file just means you don't have to worry about another program changing the database file when your application is using it (so you don't have to write any logic to make sure no other program is writing to the database file when your application wants to read/write to the database file). But that has nothing to do with the number of Data instances in an application. If each thread has its own Data instance, marking all methods synchronized in Data class will not have any effect.

Pawel Pawlowicz wrote:
You are right. But I think this is a problem for a business layer, not the Data class.

It's indeed a problem which is solved in the business layer. And the problem scenario you described can be solved by creating an extra method (in your own interface) which returns record numbers and records together (e.g. in a map).

[Paweł Baczyński]

Roel De Nijs wrote:If each thread has its own Data instance, marking all methods synchronized in Data class will not have any effect.

I'll make sure that it will never happen. But I don't intend to make Data class a singleton.

[Paweł Baczyński]

Pawel Pawlowicz wrote:I'll make sure that it will never happen. But I don't intend to make Data class a singleton.

Yeah, I just quoted myself :P.
I have a question. I know I will use my Data class in a proper way. That means I won't create two (or more) instances of this class (per a database file -> and there is only one file) in my project.
Do I have to ensure anyone else trying to reuse that code (Data class) does this in such proper way?
Or is it sufficient to write "NEVER EVER create more than one instance per database file" in class' javadoc?

[Roel De Nijs]

In my application wrong usage of API is blocked by throwing some runtime exception. But that's not always possible and then mentioning the correct usage in the API (and of course in choices.txt) will do just fine.