Replace old Hashtable with modern data structures. #624
Conversation
|
|
I'm not sure I approve of this: it's not fixing any sort of bug nor is it likely fixing any serious performance issue. |
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; |
There was a problem hiding this comment.
I'm fairly certain this doesn't meet Google Java style that we utilize.
You should run mvn formatter:format.
| config.put(key, newconf.get(newkey)); | ||
| } | ||
| newconf.forEach((key, value) -> config | ||
| .put(key.equals("PubkeyAcceptedKeyTypes") ? "PubkeyAcceptedAlgorithms" : key, value)); |
There was a problem hiding this comment.
I would rather just see a traditional for loop over an newconf.entrySet() instead of the .forEach() lambda.
|
|
||
| public class JSch { | ||
| /** The version number. */ | ||
| public static final String VERSION = Version.getVersion(); | ||
|
|
||
| static Hashtable<String, String> config = new Hashtable<>(); | ||
| static Map<String, String> config = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
I don't think there is any value in using a ConcurrentHashMap instead of a regular HashMap since all accesses to the config are synchronized on config anyway.
| private static Hashtable<Session, byte[]> faked_cookie_pool = new Hashtable<>(); | ||
| private static Hashtable<Session, byte[]> faked_cookie_hex_pool = new Hashtable<>(); | ||
| private static final Map<Session, byte[]> faked_cookie_pool = new ConcurrentHashMap<>(); | ||
| private static final Map<Session, byte[]> faked_cookie_hex_pool = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
I don't think there is any value in using a ConcurrentHashMap instead of a regular HashMap since all accesses to the faked_cookie_hex_pool are synchronized on faked_cookie_hex_pool anyway.
| @@ -45,8 +46,8 @@ class ChannelX11 extends Channel { | |||
| static byte[] cookie = null; | |||
| private static byte[] cookie_hex = null; | |||
|
|
|||
| private static Hashtable<Session, byte[]> faked_cookie_pool = new Hashtable<>(); | |||
| private static Hashtable<Session, byte[]> faked_cookie_hex_pool = new Hashtable<>(); | |||
| private static final Map<Session, byte[]> faked_cookie_pool = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
I would just keep faked_cookie_pool a HashMap and maintain the synchronized on faked_cookie_pool that was removed anyway.
| @@ -78,7 +79,7 @@ public void setXForwarding(boolean enable) { | |||
| @Deprecated | |||
| public void setEnv(Hashtable<byte[], byte[]> env) { | |||
| synchronized (this) { | |||
| this.env = env; | |||
| this.env = new ConcurrentHashMap<>(env); | |||
There was a problem hiding this comment.
I don't think there is any value in using a ConcurrentHashMap instead of a regular HashMap since all accesses to the env are synchronized on env anyway.
This PR replaces the old Hashtable with modern data structures. Hashtable is no longer maintained and is not optimised for better performance.
I replaced type declaration with the interface. Since Hashtable is-a Map, this is seamless.
I replaced Hashtable with ConcurrentHashMap where multiple threads is expected to access the map, but ConcurrentHashMap handles it better (this could be improved further, but one step at a time).
I replaced Hashtable with HashMap where synchronisation was unnecessary.