Work at SourceForge, help us to make it a better place! We have an immediate need for a Support Technician in our San Francisco or Denver office.

Close

#80 ConnectionProtocol recursive loop for onStop()

open
nobody
J2SSH (50)
6
2009-07-14
2009-07-14
Stefan Lazar
No

while testing our implementation we ran into an issue with a thread hanging. We had a small correction in class:
com.sshtools.j2ssh.connection.ConnectionProtocol

The problem is created when the server is closing the socket due to time-out and this exception is thrown (expected behaviour):
throw new IOException("The socket is EOF");

while trying to close the connections the method: onStop() has a loop that iterates over the active channels, trying to remove them from the collection. This would exhibit a ConcurrentException which was hidden somewhere in the code due to an un-handled catch Exception. This correction worked for us: a clone collection was provided for looping. Initial code was commented.

Sincerely,
Stefan Lazar

protected void onStop() {
log.info("Closing all active channels");
try {
Channel channel;
//
//SL
// Make a collection copy to avoid ConcurrentException due
// to remove items while looping
Collection cloneActiveChannels = new ArrayList(3);
for (Iterator x = activeChannels.values().iterator(); x.hasNext();) {
cloneActiveChannels.add( x.next() );
}
// Now use the clone collection
for (Iterator x = cloneActiveChannels.iterator(); x.hasNext();) {
channel = (Channel) x.next();
if (channel != null) {
if (log.isDebugEnabled()) {
log.debug("Closing " channel.getName() " id="
String.valueOf(channel.getLocalChannelId()));
}
channel.close();
}
}
// for (Iterator x = activeChannels.values().iterator(); x.hasNext();) {
// channel = (Channel) x.next();
//
// if (channel != null) {
// if (log.isDebugEnabled()) {
// log.debug("Closing " channel.getName() " id="
// String.valueOf(channel.getLocalChannelId()));
// }
//
// channel.close();
// }
// }
} catch (Throwable t) {
log.error("Unable to close all channels: " t.getMessage(),t);
}
activeChannels.clear();
}

Discussion

  • Stefan Lazar
    Stefan Lazar
    2009-07-14

    • priority: 5 --> 6
     
  • Stefan Lazar
    Stefan Lazar
    2009-07-14

    1. the recursion traverses these methods:
    ConnectionProtocol.onStop() -> iterates over activeChannels collection -> channel.close() -> channel.finalizeClose() -> (back to initial class:) ConnectionProtocol.freeChannel() -> remove from activeChannels what we're iterating over!!
    2. Original code has final catch in onStop() empty, so the error is hidden.