CODEWITHSUNDEEP
javasynchronized
Babu Patil
Babu Patil

Reputation: 31

Thread safety on block of code which has external reference

I have question regarding thread safety. The below code which is inside the synchronisation block has external method call (httpClient.execute()) on class variable reference of the current object. Is this external call causes thread safety issue? If yes how to resolve this?

public class KeyClient {

    private final HttpClient httpClient;

    public getKey() {
        synchronized (this) {
            if (Paths.get("fileName").toFile().exists()) {
                if (file == null) {
                    file = new File("fileName");
                }
            } else {
                HttpResponse response = httpClient.execute();
                Map map = mapper.readValue(response.getResponseBody(), Map.class);
                file = new File("fileName");
                mapper.writeValue(file, map);
            }
        }
    }
}

public class HttpClient {

    public HttpResponse execute() {
        //Some code
        return new HttpResponse(200, "");
    }
}

Upvotes: 1

Views: 114

Answers (5)

michid
michid

Reputation: 10814

The synchronized (this) statement ensures that no other thread will enter the subsequent block while one thread already entered it. From this point of view the code is thread safe.

However, nothing protects httpClient from being used from another non synchronized method on the KeyClient class that is called from another thread.

Finally, there is always a potential for introducing deadlocks when synchronizing code that is beyond ones one control: should httpClient.execute() at some point call back into your code, which in turn through a chain of complicated dependencies tries to aquire the monitor on KeyClient. As it stands this is not a problem here. However, these sort of problems tend to be introduced once the code evolves and it is worth thinking of them early on.

Upvotes: 0

Oresztesz
Oresztesz

Reputation: 2430

You have two possible shared resources that you have to pay attention to. Both of them can compromise thread safety.

  • HttpClient instance inside KeyClient: Even though it's marked as final, the KeyClient should be responsible for creating HttpClient in the constructor. You should not share HttpClient instance with any other class. Also, it's a good rule of thumb to check thread safety for each method of each class you're sharing in their JavaDoc.

  • The file with "fileName" should be also considered as a shared resource, so be aware that another thread might write to the same file at the same time which will mess up your data. Ensure that each thread writes to its own file at the same time. A good approach is to generate a unique file name prefix on-the-fly when file is written. For example, it can be thread-ID-process-ID-timestamp-fileName.txt

Upvotes: 0

Alexander Pavlov
Alexander Pavlov

Reputation: 2210

Only one thread can enter synchronized section so your http client instance is never used concurrently. Also, synchronized section makes any entering/leaving thread to sync its state with other thread so it does not matter if http client has mutable state because it will be properly synchronized.

So code is thread safe.

Upvotes: 0

Srinivasan Sekar
Srinivasan Sekar

Reputation: 2119

No it will not.

If you call synchronized block whatever within the block also get locked.The lock is enabled when you enter a synchronized block and is disabled when you exit that block.

However calls to execute method by other threads are not locked -- anyone can call them at the same time.

Upvotes: 3

Blokje5
Blokje5

Reputation: 4993

The HTTPClient is final, which means it is a constant variable and it can't be changed after it is initialised. This code therefore most likely does not have any thread safety issues (unless of course there are mutable variables inside of the HTTPClient class that are unsynchronised).

Upvotes: 0

Related Questions