CODEWITHSUNDEEP
javamultithreadingsynchronization
user1563047
user1563047

Reputation: 51

java locking on an object instance ... is this safe?

My question is regarding locking on an object instance in Java. I've got the following method that can be called by multiple threads at the same time. I've chosen to lock on an object instance since i'd like to allow concurrent processing as long as one instance is being accessed only by one thread at a time.

My simplified code goes something like this where I am basically locking on a local variable. Would this really be satisfying what I need? I've been reading suggestions never to lock on an object that might change and i'm suddenly unsure whether this is what i'm doing after all!

Thanks!

Edit:

Oh dear ... I'm just realizing that my attempt to simplify my code for posting might be a bit misleading.

I am renaming the methods I previously called "getInstance" to something else ("getFromMap") to demonstrate that the method call is custom code which returns a reference to an object in a hashmap.

Would the previous answer still hold? apologies for confusion!

public boolean processInput(...) {
if(message == 1) {
    Class_0 context = (Class_0)Class_0.getFromMap("xyz");
    synchronized(context) {
        context.setContextParams("abc");
       context.evaluateContextRules(message, this);
    }
} else if(message == 2) {
    Class_1 context = (Class_1)Class_1.getFromMap("efg");
    synchronized(context) {
        context.setContextParams("abc");
        context.evaluateContextRules(message, this);
    }
}
.....
}

Upvotes: 2

Views: 434

Answers (5)

Jonas Eicher
Jonas Eicher

Reputation: 1513

As I understand it, your method processInput is being called and your synchonized blocks make sure, that context.evaluateContextRules(...) runs only in one thread.

If that's your goal, then yes, your code does that.

Others mentioned that you have to make sure that you are locking the object, not the reference to the object. Well, as it looks in your code, you are synchronizing on the very same object that you call a method from, so that shouldn't be an issue.

Upvotes: 0

Stephen C
Stephen C

Reputation: 718886

There is not really enough "context" (pun not intended) in your question to be sure if this code is entirely thread-safe.

However, if we assume that 1) message is a local variable, and 2) everything that accesses or updates a context object does so while synchronizing on the object, then the code in this fragment is thread-safe.

I've been reading suggestions never to lock on an object that might change and i'm suddenly unsure whether this is what i'm doing after all!

What you should be worried about is something like this:

public class Foo {
    // some state variables

    public Object lock = new Object();

    public void doSomething(...) {
        synchronized (lock) {
            ....
        }
    }
}

This is potentially unsafe. If something change the value of lock at just the right time, and two operations on the same Foo instance could end up synchronizing using different lock objects. That could result in operations involving the Foo state not synchronizing properly.

In your example, the synchronization on context seems to be for operations on the context object itself, so the problem above does not apply.

Upvotes: 1

ekholm
ekholm

Reputation: 2573

Your code will work as long as the modifications you do to context does not affect what will be returned by the getInstance method

Upvotes: 0

Ernest Friedman-Hill
Ernest Friedman-Hill

Reputation: 81694

A variable of object type is just a reference to an object; it's the object that is locked, not the reference. A lock like this is effective if -- and only if -- all the clients that might read or write the relevant data all agree to do so only when the hold the lock on that same object. Whether it's a local variable or not doesn't matter -- it's whether everybody is using the same object as the lock.

Here, your objects seem to be coming from some kind of factory; if getInstance(X) always returns the same object for a given value of X, you're probably good.

Upvotes: 3

Brian Agnew
Brian Agnew

Reputation: 272297

Assuming that:

Class_1 context = (Class_1)TestForInternals_0.getInstance("efg");

simply retrieves a reference to an existing object and doesn't create a new instance, then that object's monitor will be locked and what you're doing is valid.

Upvotes: 1

Related Questions