CODEWITHSUNDEEP
javamultithreadingconcurrencycoding-style
Dijkstra
Dijkstra

Reputation: 333

In java is passing local objects to threads bad style?

I find I do something like the following often. Make a copy of an object to send to a thread. The thread is the only one to ever use the object and we have a happens-before relationship so it's thread safe.

But this makes me feel nervous. As the comment states, what if someone comes along and diddles with objForThread? Should I be using locks? Or is this a common accepted java pattern?

class Example
{
  private SomeObj mDynamicObj = new SomeObj();

  public void doWorkInAThread()
  {
    mutateThis(mDynamicObj);
    final SomeObj objForThread = new SomeObj(mDynamicObj);

    myExecutorService.submit(new Runnable() { @Override public void run()
    {
      doSomethingWith(objForThread);
    }});

    mutateThis(mDynamicObj);

    // Concerned that in the future someone will come
    // along and mutate objForThread here making this thread unsafe
  }
}

Upvotes: 1

Views: 148

Answers (3)

Stephen C
Stephen C

Reputation: 718788

Defending your code against people making changes that might break it is difficult.

You could argue it is not your problem. You could add warning comments and move on. Or you could assume that the next guy is smart / careful enough to figure out the implications of his changes.

Or you could take defensive steps like changing SomeObj to be thread-safe or even immutable ... even though this might add extra runtime overheads.

Which approach best? I don't think I can advise on that. It depends on higher level issues; e.g. how good is the team and its code-review and testing procedures, how complex is the overall application, how critical is performance, how critical are errors, and so on.

For this particular "example", you have abstracted away anything that resembles application logic, and it makes it hard to know what approach is best. Whether passing a local object is "good" or "bad" depends on the context.

Upvotes: 0

ATrubka
ATrubka

Reputation: 4004

It all depends on what you're trying to achieve. Some data is designed to be accessed and modified by multiple threads. Some data is designed to be used in thread safe environment only.

If you need a more detailed explanation, you have to provide a real life example.

Additionally, some classes are designed to be thread safe or immutable. For instance, having String or Integer shared is perfectly fine. However, actual references to those objects may change, so if you rely on such a reference, then you may have a problem.

In your example the latter is not the case because you reference object through final variable. However, if you were to reference member variable mDynamicObject, then you would have a problem if someone in another thread assigned a different object to it (mDynamicObject = new SomeObj()). Without proper synchronization it could've lead to a weird state of your application. To avoid that you can assign it to a final variable and reference as such.

Consider passing as much as you can as parameters to new thread invocation instead of having them referenced. This will guarantee you that referenced will not be changed.

And of course objects themselves better be immutable or properly synchronized if needed.

Upvotes: 0

Mikhail Vladimirov
Mikhail Vladimirov

Reputation: 13890

If you feel nervous, you better pass reference to thread and not keep it local:

class Example
{
    private SomeObj mDynamicObj = new SomeObj ();

    public void doWorkInAThread ()
    {
        class MyRunnable implements Runnable
        {
            private final SomeObj objForThread;

            public MyRunnable (SomeObj objForThread)
            {
                this.objForThread = objForThread;
            }

            @Override
            public void run ()
            {
                doSomethingWith (objForThread);
            }
        }

        mutateThis (mDynamicObj);

        myExecutorService.submit (new MyRunnable (new SomeObj (mDynamicObj)));

        mutateThis (mDynamicObj);
    }
}

Upvotes: 1

Related Questions