Odd Multi-thread behavior when refactoring

Discussion in 'Java' started by Christian Bongiorno, Jun 21, 2004.

  1. 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);
    }
    }
    }
     
    Christian Bongiorno, Jun 21, 2004
    #1
    1. Advertising

  2. 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
     
    Thomas Weidenfeller, Jun 22, 2004
    #2
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Elliot M. Rodriguez

    PLEASE HELP = odd TextChanged behavior

    Elliot M. Rodriguez, Oct 21, 2003, in forum: ASP .Net
    Replies:
    2
    Views:
    328
    Elliot M. Rodriguez
    Oct 22, 2003
  2. Guest

    Step-thru code - odd behavior

    Guest, May 28, 2004, in forum: ASP .Net
    Replies:
    2
    Views:
    391
    Guest
    Jun 1, 2004
  3. =?Utf-8?B?Q2hyaXM=?=
    Replies:
    1
    Views:
    338
    Karl Seguin
    Jan 7, 2005
  4. Michael Speer

    Odd behavior with odd code

    Michael Speer, Feb 16, 2007, in forum: C Programming
    Replies:
    33
    Views:
    1,137
    Richard Heathfield
    Feb 18, 2007
  5. liu yang
    Replies:
    4
    Views:
    2,024
    Antoninus Twink
    Jul 28, 2008
Loading...

Share This Page