0

I have a MyService class, which has only one public method doTask(), I use synchronized keyword to keep it being accessed in a thread-safe manner:

public class MyService {

 String myTaskId;
 public MyService {
   myTaskId = getTaskId();
 }

 public synchronized void doTask() {
   myTaskId = getTaskId();
   ...
 }

 private String getTaskId() {
   ...
 }
}

There is a private function getTaskId() which is invoked both in constructor and in doTask() function. I am wondering is it worthy to have synchronized keyword also on getTaskId() function?

Leem.fin
  • 40,781
  • 83
  • 202
  • 354
  • Yes; just in case you add another non-synchronized method which also calls `getTaskId()` in the future. Note that you should also synchronize the assignment in the constructor, to guarantee the visibility of the value you assign. – Andy Turner May 31 '16 at 15:46
  • @AndyTurner , but is it necessary to have synchronized block in constructor? I mean only after MyService is fully constructed, the `doTask()` is able to be called right? Could you please explain in what scenario, adding synchronized block in constructor is needed? Or are you thinking the `MyService` is constructed in one thread while the same instance is accessed on the other thread? I get confused on this issue now. – Leem.fin May 31 '16 at 15:51
  • 2
    Because `myTaskId` is not final, the JVM is free to reorder the return of the constructor and the assignment of `myTaskId`, meaning that some threads could read the value before assignment. You need the `synchronized` block to force the assignment to happen before constructor return. – Andy Turner May 31 '16 at 15:53
  • rare & complicated ones.. http://shipilev.net/blog/2014/safe-public-construction/ is imo a fairly good explanation - "only after MyService is fully constructed" is exactly not a thing. ps: `volatile` field instead of `synchronized` in the constructor should also do. – zapl May 31 '16 at 15:53
  • 2
    This depends somewhat on what `getTaskId` actually does. Does it access shared (e.g. static) state? Your code example shows it computing a value seemingly out of thin air which suggests it might access some shared state. – Radiodef May 31 '16 at 16:12
  • if this service is accessed concurrently i don't get the point of storing the task id in an instance variable. – Nathan Hughes May 31 '16 at 16:13
  • With the example you have, there is no _need_ to declare the private method `synchronized`. However, your example is clearly incomplete. The question asks "_should_ I use synchronized" which is somewhat subjective, especially without further information. – davmac May 31 '16 at 18:01
  • @Andy Turner: seeing a not fully constructed object can only happen when the object is not properly published. Assuming incorrect usage of your object and protecting against broken code is rarely a good programming strategy. – Holger Jun 06 '16 at 18:24

3 Answers3

0

Your example code doesn't show any variables. People talk about synchronizing methods, but the purpose of synchronization has nothing to do with methods: It's all about protecting shared data.

Where I work, it's very common to see synchronized private methods and/or synchronized blocks within private methods. We do it whenever it is possible for two or more threads to enter the method/block at the same time to operate on the same data.

Knowing that a method is private does not tell you anything about what threads might call it. It only tells you that the immediate caller will be somewhere in the same class.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
-1

This question is line with another question on stack overflow. If a synchronized method calls another non-synchronized method, is there a lock on the non-synchronized method

Since getTaskId is non synchronized there is no lock created when a synchronized method calls it. But again as it is private lock does not create any effect.

To answer your question, as long it stays private and all the method that call this method are synchronized you do not have to explicitly imply the same.

Community
  • 1
  • 1
Nithyakumaran Gnanasekar
  • 1
  • But as you see, my constructor also calls it, so, your answer is still not as clear as i expected. – Leem.fin May 31 '16 at 15:50
  • That is subjective question. I would mark the getTaskId synchronized if there is chance of reading a stale object. If taskId is static value that doesnt change much, I wouldnt bother adding synchronized keyword. – Nithyakumaran Gnanasekar May 31 '16 at 15:59
-1

when the object is creating,the constructor method will be invoked first.After that,method doTaskk() can be invoked.So it is not necessary to add synchronize on getTaskId

Ares
  • 1