Odd Multi-thread behavior when refactoring

[Christian Bongiorno]

So, I am working on someone elses code. He has some clearly C&P code
that I have attempted to refactor. However, when I refactor the code,
I get, amongst other things: ConcurrentModificationException

Depending upon the run, I get NPE or ClassCastException.

There are many threads in this system, and as far as I can tell the
code is very "unsafe" for threads. Howevever, what I can't understand
is how it ever ran in the first place! The code I am refactoring is in
the same method -- one right below the other. The synchronized keyword
doesn't even show up in this project so I know that was a never a
consideration.

Is there something about the JVM/OS and how it handles threads that
would make it thread safe if access was in one method call?? I suppose
I could see the JVM swapping a thread out when a method is called

Here is the code, along with my refactor
------------------------begin code--------------------------
for (int i = 1; i < info.devicesBySlotinFront.size(); i++) {
DeviceInfo deviceInfo = (DeviceInfo)
info.devicesBySlotinFront.get(i);

if (deviceInfo != null &&
!this.deviceInfos.contains(deviceInfo)) {
DeviceDisplayPanel newDevice = new
DeviceDisplayPanel(this, deviceInfo);
devices.add(newDevice);
deviceInfos.add(deviceInfo);
deviceInfo.addListener(newDevice);

int x = RACK_BORDER_WIDTH;
if (!deviceInfo.isFront) {
x +=
DeviceDisplayPanel.DEVICE_PIXEL_WIDTH;//(int)(DeviceDisplayPanel.BORDER_WIDTH
/ 2);
}
newDevice.setLocation(x,
-RACK_BORDER_WIDTH +
this.rackPanel.getHeight() - deviceInfo.slot *
DeviceDisplayPanel.PIXELS_PER_SLOT);


this.rackPanel.add(newDevice);
}
}

for (int i = 1; i < info.devicesBySlotinBack.size(); i++) {
DeviceInfo deviceInfo = (DeviceInfo)
info.devicesBySlotinBack.get(i);

if (deviceInfo != null &&
!this.deviceInfos.contains(deviceInfo)) {
DeviceDisplayPanel newDevice = new
DeviceDisplayPanel(this, deviceInfo);
devices.add(newDevice);
deviceInfos.add(deviceInfo);
deviceInfo.addListener(newDevice);

int x = RACK_BORDER_WIDTH;
if (!deviceInfo.isFront) {
x +=
DeviceDisplayPanel.DEVICE_PIXEL_WIDTH;//(int)(DeviceDisplayPanel.BORDER_WIDTH
/ 2);
}
newDevice.setLocation(x,
-RACK_BORDER_WIDTH +
this.rackPanel.getHeight() - deviceInfo.slot *
DeviceDisplayPanel.PIXELS_PER_SLOT);


this.rackPanel.add(newDevice);
}
}
------------------------end code--------------------------

--------------------------Begin Refactor---------------
addDevices(info.devicesBySlotinFront);
addDevices(info.devicesBySlotinBack);
....
// refactored method
private void addDevices(Vector device) {
for (Iterator i = device.iterator(); i.hasNext(); ) {
DeviceInfo deviceInfo = (DeviceInfo) i.next();

if (deviceInfo != null &&
!this.deviceInfos.contains(deviceInfo)) {
DeviceDisplayPanel newDevice = new
DeviceDisplayPanel(this, deviceInfo);
devices.add(newDevice);
deviceInfos.add(deviceInfo);
deviceInfo.addListener(newDevice);

int x = RACK_BORDER_WIDTH;
if (!deviceInfo.isFront) {
x +=
DeviceDisplayPanel.DEVICE_PIXEL_WIDTH;//(int)(DeviceDisplayPanel.BORDER_WIDTH
/ 2);
}
newDevice.setLocation(x,
-RACK_BORDER_WIDTH +
this.rackPanel.getHeight() - deviceInfo.slot *
DeviceDisplayPanel.PIXELS_PER_SLOT);


this.rackPanel.add(newDevice);
}
}
}

 

[Thomas Weidenfeller]

So, I am working on someone elses code. He has some clearly C&P code
that I have attempted to refactor. However, when I refactor the code,
I get, amongst other things: ConcurrentModificationException

Yep, the original author maybe knew what he was doing? You changed his
for loop to an iterator, and collections don't like to be changed while
iterated. You didn't refactor the code, you broke it.

There are many threads in this system, and as far as I can tell the
code is very "unsafe" for threads. Howevever, what I can't understand
is how it ever ran in the first place! The code I am refactoring is in
the same method -- one right below the other. The synchronized keyword
doesn't even show up in this project so I know that was a never a
consideration.

There is much more to thread save programming than just throwing
synchronized keywords in the code. You can have thousands of lines of
code which are thread save, and don't at all need a single synchronized
anywhere.

It is impossible to judge if some code is thread save without seeing all
the code, and also without knowing the requirements and the algorithm
design and data structures.

/Thomas