Slowness due to blocked threads in Channel.getChannel call in multi-thread environment

[DavidTavoularis]

On a quite old jsch release (0.1.55), we could see lots of blocked threads in multi-thread environment.
This is due to the fact that Channel is using a Vector for its internal pool.

The issue is still there on master, so I have prepared a merge-request, pending your review and approval.

Note1: The code change was tested fine in lab and production (on top of 0.1.55).
The suggested code change had to manage this new line (not present in 0.1.55):
index &= Integer.MAX_VALUE;
which was introduced recently with 793753a
So it required some specific change to be thread-safe (and not tested in production).

Note2: After code change on top of 0.1.55, it was observed no more slowness or blocked threads

Here are the same observed stack-traces (every minute) showing the blocked thread when calling Channel.getChannel:
Stack 4 - Mar 05 05:54:01
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004e52a0168> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

Stack 5 - Mar 05 05:55:00
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004e52a0168> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

Stack 6 - Mar 05 05:56:00
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004e52a0168> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

Stack 7 - Mar 05 05:57:01
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004e52a0168> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

Stack 8 - Mar 05 05:58:00
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004e52a0168> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

[...]

Stack 28 - Mar 05 06:18:00
"Connect thread X.X.X.X session" #2631 prio=5 os_prio=64 tid=0x0000000106346800 nid=0xa51 waiting for monitor entry [0xfffffffe31bfe000]
java.lang.Thread.State: BLOCKED (on object monitor)
at com.jcraft.jsch.Channel.getChannel(Channel.java:83)
- waiting to lock <0x00000004d6640128> (a java.util.Vector)
at com.jcraft.jsch.Session.run(Session.java:1448)
at java.lang.Thread.run(Thread.java:745)

[DavidTavoularis]

@norrisjeremy Indeed, I prefer this new approach

1. Regarding getChannelById() method (Session.java:1142-1155), should you replace "c.id == id && c.session == this" by "c.id == id" as session check is redundant since channels are now stored per-session

2. Regarding delChannel() method (Session.java:2111-2118), should you just do "channels.remove(c)" instead of using an iterator ?

3. Regarding disconnect() method (Session.java:2137-2146), should you avoid iterator ?
   from :

Iterator<Channel> it = channels.iterator();
      while (it.hasNext()) {
        it.next()._disconnect();
        it.remove();
      }

to :

      for (Channel channel : channels) {
        channel._disconnect();
      }
      channels.clear();

4. Why did you add "_" in _disconnect method name (cf SonarQloud) ?

5. Regarding your concern, one solution would be to add "volatile boolean disconnecting = false;" in Channel.java and "if (c.id == id && !c.disconnecting)" in Session.getChannelById method and perform this change in Session.disconnect() method:

    // Mark all channels as disconnecting
    List<Channel> channelsToDisconnect;
    Lock l = channelsLock.writeLock();
    l.lock();
    try {
      channelsToDisconnect = new ArrayList<>(channels);
      for (Channel channel : channels) {
        channel.disconnecting = true;
      }
    } finally {
      l.unlock();
    }

    // Disconnect channels without holding the lock (I/O operations)
    for (Channel channel : channelsToDisconnect) {
      channel._disconnect();
    }
    
    // Remove all channels after disconnection is complete
    l = channelsLock.writeLock();
    l.lock();
    try {
      channels.clear();
    } finally {
      l.unlock();
    }

This way, no I/O under lock (blocking operations happen outside the critical section), thread-safety (Channels remain in list during disconnect but are marked as unavailable), consistent state (getChannelById() won't return disconnecting channels) and no deadlock risk (lock is only held for quick memory operations)

[norrisjeremy]

Hi @DavidTavoularis,

Can you move the comments you left here about #889 and instead put them directly inline into the PR #889?
It will make it much easier for us to converse and track the changes.

Thanks!
Jeremy