Win a copy of Murach's Python Programming this week in the Jython/Python forum!
  • Post Reply Bookmark Topic Watch Topic
  • New Topic

synchronized doesn't work as expected  RSS feed

 
Sergey Izg
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Hi,
Here is the simple synchronization testing code:



I expect to see output 500, but it's not the case.
Could you please help understand what am i doing wrong here?
 
Henry Wong
author
Sheriff
Posts: 22832
119
C++ Chrome Eclipse IDE Firefox Browser Java jQuery Linux VI Editor Windows
  • Likes 2
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sergey Izg wrote:
I expect to see output 500, it's not the case.
Could you please help understand what am i doing wrong here?


First, welcome to the ranch.

You have two issues. One. Your getInstance() method is not thread safe. It is possible for different threads to have different data instances. Two. Your updateUnit() method is not thread safe. The operation is not atomic, and it is this issue that is causing your result to be less than expected.

Henry
 
Stevens Miller
Bartender
Posts: 1443
30
C++ Java Netbeans IDE Windows
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Henry Wong wrote:
Sergey Izg wrote:
I expect to see output 500, it's not the case.
Could you please help understand what am i doing wrong here?


First, welcome to the ranch.

You have two issues. One. Your getInstance() method is not thread safe. It is possible for different threads to have different data instances. Two. Your updateUnit() method is not thread safe. The operation is not atomic, and it is this issue that is causing your result to be less than expected.

Henry


I think Henry meant this, but I'll emphasize that both methods are contributing to your problem. To help see what's wrong in your factory method, add this at Line 3, below:

You are probably going to be suprised to see that it is creating more than one instance. Each new instance is being stored in the same place, so only the last one survives to the end of your program. Any updates to the others will be lost.

Remember that each thread runs as though it were simultaneous with the others. Thus, each thread can be testing to see if instance is null at the same time. When I run your code, all five threads saw instance as null, so all five created a new instance of data.

By the way, class names should always start with a capital letter.
 
Sergey Izg
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thank you very much, guys. i made data instance be initiated before start running the threads and it did a trick
 
Paweł Baczyński
Bartender
Posts: 1997
42
Firefox Browser IntelliJ IDE Java Linux Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Those issues are called race conditions.
First one is check-then-act.

In general the task is:
1. Check a condition.
2. Act on a condition (decide what to do based on the condition).

In you case it is.
1. Check if a variable is null.
2. If it is null assign new object reference to it.

It might go like this (T1 and T2 are thread names):
T1 checks if a variable is null.
T1 sees it as null so it assigns new object reference to it.
T2 checks if a variable is null.
T2 sees is as not null so it does nothing.

Which is an execution you expect.

But it might also execute like this:
T1 checks if a variable is null.
T2 checks if a variable is null.
T1 sees it as null so it assigns new object reference to it.
T2 sees it as null so it assigns new object reference to it.

And you can observe two objects being created.

The second type of race condition is read-modify-write.

In general it goes as:
1. Read a value.
2. Modify the value.
3. Write the value.

In your case it is.
1. Read the counter from the variable.
2. Add one to the counter.
3. Write the counter to the variable.

It might go like this:
T1 reads value 15 from the counter.
T1 adds one to the value.
T1 writes 16 to the counter.
T2 reads value 16 from the counter.
T2 adds one to the value.
T2 writes 17 to the counter.

Which is an execution you expect.

But it might also execute like this:
T1 reads value 15 from the counter.
T2 reads value 15 from the counter.
T1 adds one to the value.
T1 writes 16 to the counter.
T2 adds one to the value.
T2 writes 16 to the counter.

And you can observe one increment being lost.
 
Sergey Izg
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Paweł,

But it might also execute like this:
T1 reads value 15 from the counter.
T2 reads value 15 from the counter.
T1 adds one to the value.
T1 writes 16 to the counter.
T2 adds one to the value.
T2 writes 16 to the counter.


But shouldnt it be synchronized? Should T2 not have access to the method while T1 handles it? Or, synchronized is not about read?

Thanks
 
Paweł Baczyński
Bartender
Posts: 1997
42
Firefox Browser IntelliJ IDE Java Linux Spring
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
You are right. This is another way it could execute. There are others.

Synchronized on a method guarantees an exclusive access only to one method at a time.
So, read and write are atomic as they are synchronized.

But this does not help you much.
T1 can read the value in atomic way. Once it is done T2 can read the value in atomic way before T1 updates the value.

In pseudocode you currently have: ...which is not atomic. Something might happen in other thread between those synchronized blocks.

You need the entire read-modify-write sequence to be atomic.

One way of ensuring that is to provide one synchronized method that does the job.
In pseudocode it would be:

Other way is to use a specialized thread-safe class like AtomicInteger or LongAdder.
 
Sergey Izg
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
So, will the below be more right?

 
Paweł Baczyński
Bartender
Posts: 1997
42
Firefox Browser IntelliJ IDE Java Linux Spring
  • Likes 1
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
That will do the job.

Please, don't use [strike] tags inside [code] tags.
I have changed them to block comments this time.
 
Sergey Izg
Greenhorn
Posts: 5
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Thanks
 
Stevens Miller
Bartender
Posts: 1443
30
C++ Java Netbeans IDE Windows
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Very nice guide for a Greenhorn, Paweł. Have another cow for your herd.
 
Paweł Baczyński
Bartender
Posts: 1997
42
Firefox Browser IntelliJ IDE Java Linux Spring
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Also, you could have unit.updateUnit() synchronized and this would not require changes to data class.
 
Winston Gutkowski
Bartender
Posts: 10573
65
Eclipse IDE Hibernate Ubuntu
  • Mark post as helpful
  • send pies
  • Quote
  • Report post to moderator
Sergey Izg wrote:Or, synchronized is not about read?

I think Pawel's summed it up for you, but there are many things that simple synchronization can't guard against. It is an exclusion protocol, pure and simple, but it does NOT guarantee the order in which excluded Threads execute.

It also only works on whatever action is done in the method, so it can't, for example, lock out an entire dataset while you sort it, unless you also synchronize the sort() method.

IMO, it's almost too simple because, while it guarantees method atomicity, it doesn't guarantee functional atomicity. That's your job ... and unfortunately, it's easy to get wrong.

You might also want to read about ReentrantLocks (←click), because they provide quite a bit more control (and possibly speed) - albeit at the expense of simplicity.

Good luck.

Winston
 
  • Post Reply Bookmark Topic Watch Topic
  • New Topic
Boost this thread!