Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > Java > Odd Multi-thread behavior when refactoring

Reply
Thread Tools

Odd Multi-thread behavior when refactoring

 
 
Christian Bongiorno
Guest
Posts: n/a
 
      06-21-2004
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);
}
}
}
 
Reply With Quote
 
 
 
 
Thomas Weidenfeller
Guest
Posts: n/a
 
      06-22-2004
Christian Bongiorno wrote:
> 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
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Odd behavior with odd code Michael Speer C Programming 33 02-18-2007 07:31 AM
VERY odd routing behavior when attempting VPN connections over Wifi Robert Gordon Wireless Networking 0 08-25-2005 04:04 PM
Firefox under Linux -- odd behavior Dennis J. Tuchler Firefox 0 07-28-2004 04:05 PM
Odd behavior behind the PIX Charles Haron Cisco 1 04-21-2004 04:05 AM
Odd console behavior on Cat 5005 Mike Voss Cisco 0 11-19-2003 10:49 PM



Advertisments