Inspector Confusion

I have a couple of Inspector warnings popping up that seem to stem from "disagreements" between myself and the IDEA developers on what constitutes ideal coding practice. I'm posting to see if I can get a bit of clarification on the motives behind these warnings. To be clear, this is more of a "Java best practices" discussion than a discussion on IDEA itself, but I figure there are a few Java developers on these boards, right? :)

The first case is partially documented in this thread: http://intellij.net/forums/thread.jspa?forumID=27&tstart=2&threadID=158553&trange=15

Inspector seems to complain anytime you explicitly lock on a non final object. For example, in the following code, I'd get a warning on the 'synchronized(this.requestTimes)' line.

public class MockCommunicator implements Runnable
{
private Vector requestTimes;
private Thread worker;

public MockCommunicator()
{
this.requestTimes = new Vector();
this.worker = new Thread(this);
this.worker.start();
}

public void makeRequest()
{
synchronized(this.requestTimes)
{
this.requestTimes.add(new GregorianCalendar());
}
}

public void run()
{
//Do some work which locks and accesses requestTimes
}
}


Note that the private vector is well encapsulated. I can understand the danger of exposing objects on which you are locking to users of a class. However, it seems to me to be messy to declare a whole other object for locking when we the structure we are protecting is private and well encapsulated. Are there other reasons for this warning?

The second case is illustrated by the following class, which uses a thread to do work, but exposes itself to the client as a synchronous worker. Note this isn't the most realistic example and I left out a bit of synchronization, but I have real world cases where this type of situation comes up.


public class MockWorker implements Runnable
{
private final Object resultReceived = new Object;
private Result result;

private Thread worker;

public MockWorker()
{
this.worker = new Thread(this);
this.worker.start();
}

public Result makeRequest(Request toProcess)
{
this.result = null;
//push work onto queue

this.resultReceived.wait();
return this.result;
}

public void run()
{
while (true)
{
//Pull work off queue, do work, put result into local variable result
this.resultReceived.notify();
}
}
}


Here, the Inspector complains that result can be made into a local variable, apparently not understanding or liking the fact that result is being used for communication between the two threads. Is there a specific problem this warning is targeting? Is there a problem with this strategy in general?

Thanks in advance for any thoughts on these matters.

Message was edited by:
Taft

2 comments

+
The first case is partially documented in this thread: http://intellij.net/forums/thread.jspa?forumID=27&tstart=2&threadID=158553&trange=15
+

Actually, that's about a different (and more controversial) inspection. The one you're asking about is a lot easier to explain.

+Note that the private vector is well encapsulated. I can understand the danger of exposing objects on which you are locking to users of a class. However, it seems to me to be messy to declare a whole other object for locking when we the structure we are protecting is private and well encapsulated. Are there other reasons for this warning?
+

The reason to only lock on a final is that you might reassign the variable. Since locks are held on values rather than variables, you might have one thread synchronizing on the initial value of the variable and a different thread synchronizing on the later value. Since these locks won't conflict, you could have two threads in a critical section when you didn't realize that was possible.

If you don't reassign the variable (as in the code you list), just mark it final. If you do reassign the variable, it's a subtle and intermittent bug waiting to happen, if not now then when someone attempts to modify your clever code.

+
Here, the Inspector complains that result can be made into a local variable, apparently not understanding or liking the fact that result is being used for communication between the two threads. Is there a specific problem this warning is targeting? Is there a problem with this strategy in general?+

Nope, no problem at all. The inspection you list is one of the (very few) non-thread-safe ones, and evidently isn't documented as such. I'll submit a tracking request.

--Dave Griffith

0

Thanks for the reply. I hadn't thought of the scenario you mentioned (for the first case). That makes a lot of sense.

Thanks again.

0

Please sign in to leave a comment.