2


1. Is the way to use synchronized here correct?
2. Do the randomScore is locked when one thread access to it so that other threads can not access to randomScore?
3. If only changeRandomScore() can access to randomScore variable and only one thread can access to changeRandomScore(), therefore only one thread can acccess to randomScore at a time. Is it correct? 

 import java.*; public class StudentThread extends Thread { int ID; public static int randomScore; StudentThread(int i) { ID = i; } public void run() { changeRandomScore(); System.out.println("in run"); } public synchronized void changeRandomScore() { randomScore = (int) (Math.random()*1000); } public static void main(String args[]) throws Exception { for (int i = 1;i< 10 ;i++) { StudentThread student = new StudentThread(5); student.start(); Thread.sleep(100); System.out.println(randomScore); } } }
Improve this question

3 Answers 3

Reset to default
6

You are accessing here a static variable inside of synchronized-methods of different objects. The synchronization has no real effect here, since each thread uses its own monitor (the thread object, in this case). For a shared variable you should use a shared monitor, too.

Here is a variant that is synchronized correctly:

public class StudentThread extends Thread { int ID; private static int randomScore; private static final Object scoreLock = new Object(); StudentThread(int i) { ID = i; } public void run() { changeRandomScore(); System.out.println("in run"); } public void changeRandomScore() { int tmp = (int) (Math.random()*1000); // no need to synchronize the random()-call, too. synchronized(scoreLock) { randomScore = tmp; } } public static void main(String args[]) throws Exception { for (int i = 1;i< 10 ;i++) { StudentThread student = new StudentThread(5); student.start(); Thread.sleep(100); synchronized(scoreLock) { System.out.println(randomScore); } } } }

The difference is that we are now using a common lock object (scoreLock) and use this as parameter for synchronized-blocks, and we also synchronize on this object in the main method on reading the score.

Alternatively we could also declare the method public static synchronized void changeRandomScore() (which means it uses the class-object as the monitor) and in the main-method synchronize on StudentThread.class.

Yeah, as others said: if you want to ensure that a variable is accessed only with proper synchronization, don't make it public.

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

7 Comments

Very good answer. I'd only make 'int ID' final to make it perfect ;-)
@Petro: Of course, but I didn't want to change the structure of the whole program, only correct the synchronization.
(It is not a silly question.) Yeah, as long as your program consists only of one class (and the Java API), there is no difference. (But then you could leave the access modifier away completely.) But later you'll be composing programs of multiple classes, some of them not written by you, and then you already should be accustomed to using private as a default for fields.
In most cases (when using normal non-static variables) you can simply synchronize on the object containing the variable (known as this in a method of this object). This is also the object the synchronized methods synchronize on. Only when you have special needs you need a special lock object (like here, where we could have instead used the class object of the class containing the method, or when your object contains multiple variables which you want to lock independently.)
That should be helpful: stackoverflow.com/questions/1237980/…
|
2

Correct, only one thread will be able to access the method changeRandomScore() at a time. However, that doesn't mean that multiple threads won't be able to modify the variable randomScore concurrently. It only guarantees that only one thread can execute that method at a time. That is especially true given that randomScore is public.

Edit Allow me to rephrase and take a stab at answering your "bigger question" rather than the explicit question you posed in your post. In order to guarantee synchronization and that only one thread can access randomScore at once, you must implement some sort of locking mechanism that controls all reads and writes of the variable. In order to successfully accomplish that, you must define an interface that controls access to the variable. You haven't done that in your example. You need to restrict the access to the variable by using private and then create methods that allow manipulation of the variable. You can accomplish this through method synchronization, but that may not always be the correct tool for the job.

For example this:

public void synchronized myMethod(){ //code }

Is equivalent to

public void myMethod(){ synchronized(this){ //same code } }

So what you're doing with method-level synchronization is synchronizing on the instance of the object. This is fine if you're only manipulating data members internal to the object, but this falls apart if the variables you are working with have any sort of scope outside of the object (for instance, in your case a static variable). That case, you would need some sort of other locking mechanism.

Improve this answer

9 Comments

@Chris Thompson: if only changeRandomScore() can access to randomScore variable and only one thread can access to changeRandomScore(), therefore only one thread can acccess to randomScore at a time. Is it correct?
@Kalla, correct, partially. First (and most importantly), changeRandomScore() isn't the only method that can access randomScore because randomScore is public so another thread could bypass the method and access the variable directly. Second (and less importantly because this would constitute "bad behavior"), even if it were private, a thread could use reflection to access the variable without using that method.
The sentence "if ...." may be right, but the main thread reads randomScore without any synchronization.
@ All: How can use synchronized to allow only one thread to access to randomScore in this example?
Not only can the variable be accessed directly (defeating the synchronization) but the synchronization as written is local to each instance of the object. Each individual object is threadsafe for randomScore, but together they are not since different objects can all access the static field simultaneously.
|
1

No, you don't use it right. The writing is synchronized, that's fine, but buys you nothing at all. Without the synchronizations, it'd work the same, because

  • access to int is atomic anyway
  • reading is not synchronized, so you may see stale values

You'd better never expose field needing synchronization.

Improve this answer

6 Comments

@maaartinus: sorry i dont get what you mean.
Which part? The one with the stale values is more complicated, so I'll try: Each thread may run on it's own core. Each core may have it's own copy of randomScore in it's cache. Because of synchronized, it must write it to the main memory. However, the main thread does not synchronize it's access so it may use it's own copy of the variable and may not see the written value. This not actually happening in your example means nothing at all!
And in fact, since each thread synchronizes on its own lock, they are not necessarily sharing values at all.
Somehow I saw there static on the method, but it's not there. So it's even more wrong.
@Paulo I was under the impression a synchronized method synchronizes on the object being called. As in to say synchronized void foo() { } is identical to void foo() { synchronized(this) { } } (Actually there is a small difference - the latter has one extra bytecode instruction. Functionally, though, they're the same.) So I'm not sure how to interpret your comment here.
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.