You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/07/12 09:54:16 UTC
[GitHub] [commons-vfs] toby1984 opened a new pull request, #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
toby1984 opened a new pull request, #272:
URL: https://github.com/apache/commons-vfs/pull/272
When using FileSystemManager as a singleton and performing repeated SFTP operations, SftpFileSystem instances will accumulate with every resolveFile() invocation. The cause for this is that AbstractOriginatingFileProvider#getFileSystem fails to locate the FileSystem from a previous invocation inside the TreeMap because all implementations of the SFTP IdentityProvider interface do not have equals() / hashCode() implementations and thus make calls to Arrays.deepHashcode() inside FileSystemOptions#compareTo always return different values, even if the FileSystemOption instances are actually equal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] toby1984 commented on pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
toby1984 commented on PR #272:
URL: https://github.com/apache/commons-vfs/pull/272#issuecomment-1181566744
Before patch:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] toby1984 closed pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
toby1984 closed pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
URL: https://github.com/apache/commons-vfs/pull/272
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] toby1984 commented on pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
toby1984 commented on PR #272:
URL: https://github.com/apache/commons-vfs/pull/272#issuecomment-1181715603
Thank you for the quick response!
The Maven build seems to not run checkstyle on test sources, at least I didn't get any errors when I build my branch. Maybe disabled because the other tests have lots of checkstyle violations as well ? I did use only the default profile though.
Anyway, I've fixed the issues you pointed out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory commented on a diff in pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #272:
URL: https://github.com/apache/commons-vfs/pull/272#discussion_r918874064
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/FileSystemOptionsTest.java:
##########
@@ -92,4 +96,63 @@ public void testEqualsHashCodeAndCompareTo() {
assertEquals(expected.hashCode(), actual.hashCode());
}
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMatch() {
+ for ( int mask = 0 ; mask < 8 ; mask++ ) {
+ assertSftpOptionsEqual(
+ (mask & 1) == 1 ? new File("/tmp/test.priv") : null,
+ (mask & 2) == 2 ? new File("/tmp/test.pub") : null,
+ (mask & 4) == 4 ? new byte[] {1,2,3} : null
+ );
+ }
+ }
+
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMismatch() {
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test1.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
Review Comment:
Refactor string constants that are reused into locals to make it obvious when the reuse is intended.
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/FileSystemOptionsTest.java:
##########
@@ -92,4 +96,63 @@ public void testEqualsHashCodeAndCompareTo() {
assertEquals(expected.hashCode(), actual.hashCode());
}
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMatch() {
+ for ( int mask = 0 ; mask < 8 ; mask++ ) {
+ assertSftpOptionsEqual(
+ (mask & 1) == 1 ? new File("/tmp/test.priv") : null,
+ (mask & 2) == 2 ? new File("/tmp/test.pub") : null,
+ (mask & 4) == 4 ? new byte[] {1,2,3} : null
+ );
+ }
+ }
+
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMismatch() {
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test1.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test.priv"), new File("/tmp/test1.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,4}
+ );
+ }
+
+ private static void assertSftpOptionsEqual(File privKey, File pubKey, byte[] passphrase) {
+ final SftpFileSystemConfigBuilder builder = SftpFileSystemConfigBuilder.getInstance();
+
+ final FileSystemOptions expected = new FileSystemOptions();
+ final IdentityInfo info1 = new IdentityInfo( privKey, pubKey, passphrase);
+ builder.setIdentityProvider(expected, info1 );
+
+ final FileSystemOptions actual = new FileSystemOptions();
+ final IdentityInfo info2 = new IdentityInfo( privKey, pubKey, passphrase);
+ builder.setIdentityProvider(actual,info2);
+
+ assertEquals( 0, expected.compareTo( actual ) );
+ assertEquals( expected.hashCode(), actual.hashCode() );
+ }
+
+ private static void assertSftpOptionsNotEqual(File privKey1, File pubKey1, byte[] passphrase1,
+ File privKey2, File pubKey2, byte[] passphrase2) {
+ final SftpFileSystemConfigBuilder builder = SftpFileSystemConfigBuilder.getInstance();
+
+ final FileSystemOptions expected = new FileSystemOptions();
+ final IdentityInfo info1 = new IdentityInfo( privKey1, pubKey1, passphrase1);
+ builder.setIdentityProvider(expected, info1 );
+
+ final FileSystemOptions actual = new FileSystemOptions();
+ final IdentityInfo info2 = new IdentityInfo( privKey2, pubKey2, passphrase2);
+ builder.setIdentityProvider(actual,info2);
+
+ assertNotEquals( 0, expected.compareTo( actual ) );
Review Comment:
Use the same formatting style as the rest of VFS: No spaces around parentheses.
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/FileSystemOptionsTest.java:
##########
@@ -92,4 +96,63 @@ public void testEqualsHashCodeAndCompareTo() {
assertEquals(expected.hashCode(), actual.hashCode());
}
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMatch() {
+ for ( int mask = 0 ; mask < 8 ; mask++ ) {
+ assertSftpOptionsEqual(
+ (mask & 1) == 1 ? new File("/tmp/test.priv") : null,
+ (mask & 2) == 2 ? new File("/tmp/test.pub") : null,
+ (mask & 4) == 4 ? new byte[] {1,2,3} : null
+ );
+ }
+ }
+
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMismatch() {
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test1.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test.priv"), new File("/tmp/test1.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
Review Comment:
IMO "Equal" -> "Equals".
##########
commons-vfs2/src/test/java/org/apache/commons/vfs2/FileSystemOptionsTest.java:
##########
@@ -92,4 +96,63 @@ public void testEqualsHashCodeAndCompareTo() {
assertEquals(expected.hashCode(), actual.hashCode());
}
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMatch() {
+ for ( int mask = 0 ; mask < 8 ; mask++ ) {
+ assertSftpOptionsEqual(
+ (mask & 1) == 1 ? new File("/tmp/test.priv") : null,
+ (mask & 2) == 2 ? new File("/tmp/test.pub") : null,
+ (mask & 4) == 4 ? new byte[] {1,2,3} : null
+ );
+ }
+ }
+
+ @Test
+ public void testEqualsHashCodeAndCompareToWithSftpIdentityProviderMismatch() {
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test1.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test.priv"), new File("/tmp/test1.pub"), new byte[] {1,2,3}
+ );
+
+ assertSftpOptionsNotEqual(
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,3},
+ new File("/tmp/test.priv"), new File("/tmp/test.pub"), new byte[] {1,2,4}
+ );
+ }
+
+ private static void assertSftpOptionsEqual(File privKey, File pubKey, byte[] passphrase) {
+ final SftpFileSystemConfigBuilder builder = SftpFileSystemConfigBuilder.getInstance();
+
+ final FileSystemOptions expected = new FileSystemOptions();
+ final IdentityInfo info1 = new IdentityInfo( privKey, pubKey, passphrase);
+ builder.setIdentityProvider(expected, info1 );
+
+ final FileSystemOptions actual = new FileSystemOptions();
+ final IdentityInfo info2 = new IdentityInfo( privKey, pubKey, passphrase);
+ builder.setIdentityProvider(actual,info2);
+
+ assertEquals( 0, expected.compareTo( actual ) );
+ assertEquals( expected.hashCode(), actual.hashCode() );
+ }
+
+ private static void assertSftpOptionsNotEqual(File privKey1, File pubKey1, byte[] passphrase1,
Review Comment:
Use final for parameters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] toby1984 commented on pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
toby1984 commented on PR #272:
URL: https://github.com/apache/commons-vfs/pull/272#issuecomment-1181567253
After patch:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-vfs] garydgregory commented on pull request #272: SFTP: Memory leak because AbstractFileProvider#findFileSystem fails to detect equality of SFTP FileSystemOptions
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #272:
URL: https://github.com/apache/commons-vfs/pull/272#issuecomment-1181960285
Hi @toby1984
Thank you for your update. I split this PR up into two commits. Please see git master and close this PR if your use case is satified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org