Skip to content

NetCopyClientConnectionPool refactorings#4426

Open
TranceLove wants to merge 3 commits into
TeamAmaze:release/4.0from
TranceLove:feature/net-copy-client-connection-pool-improvements
Open

NetCopyClientConnectionPool refactorings#4426
TranceLove wants to merge 3 commits into
TeamAmaze:release/4.0from
TranceLove:feature/net-copy-client-connection-pool-improvements

Conversation

@TranceLove

@TranceLove TranceLove commented Jun 18, 2025

Copy link
Copy Markdown
Collaborator

Description

  • Use LruCache
  • Remove use of CountdownLatch
  • Made NetCopyClientConnectionPool lifecycle-aware
  • PasswordUtil remove Context parameter

Issue tracker

Addresses #4081

Automatic tests

  • Added test cases

Manual tests

  • Done

Build tasks success

Successfully running following tasks on local:

  • ./gradlew assembledebug
  • ./gradlew spotlessCheck

@TranceLove

TranceLove commented Jun 18, 2025

Copy link
Copy Markdown
Collaborator Author

Pending tasks:

  • More test harness for NetCopyClientConnectionPool, for situations of multiple connection coexistence

@TranceLove TranceLove force-pushed the feature/net-copy-client-connection-pool-improvements branch 9 times, most recently from 4d72c46 to d71fc85 Compare June 22, 2025 04:29
@TranceLove TranceLove force-pushed the feature/net-copy-client-connection-pool-improvements branch 3 times, most recently from f9f5506 to 26dcfb5 Compare March 5, 2026 16:03
@TranceLove TranceLove marked this pull request as ready for review March 5, 2026 16:09
@TranceLove TranceLove requested a review from Copilot March 5, 2026 16:09

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors remote-connection handling and password encryption utilities, primarily to improve connection reuse/canonicalization and simplify password encryption APIs, while updating affected call sites and tests.

Changes:

  • Refactor NetCopyClientConnectionPool to use an LRU-backed cache keyed by canonicalized/base URIs and add lifecycle hooks.
  • Remove Context parameter from PasswordUtil APIs and update SMB/FTP/SFTP code paths plus tests accordingly.
  • Add CI workflow to publish unit test results from CI artifacts.

Reviewed changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
testShared/src/test/java/com/amaze/filemanager/test/ShadowPasswordUtilTest.java Update tests for new PasswordUtil signature (no Context).
testShared/src/test/java/com/amaze/filemanager/test/ShadowPasswordUtil.kt Update Robolectric shadow to match new PasswordUtil API signature.
gradle/libs.versions.toml Add AndroidX Lifecycle dependency versions.
gradle.properties Disable Gradle configuration cache via property.
app/src/test/java/com/amaze/filemanager/utils/SmbUtilTest.kt Update SMB util tests for new SmbUtil/PasswordUtil signatures.
app/src/test/java/com/amaze/filemanager/ui/dialogs/SmbConnectDialogTest.kt Update SMB dialog tests for new SmbUtil signature.
app/src/test/java/com/amaze/filemanager/ui/dialogs/SftpConnectDialogSshTest.kt Update password encryption/decryption calls for new PasswordUtil API.
app/src/test/java/com/amaze/filemanager/ui/dialogs/SftpConnectDialogFtpTest.kt Update password decryption call sites for new PasswordUtil API.
app/src/test/java/com/amaze/filemanager/ui/dialogs/SftpConnectDialogArgumentPopulationTest.kt Update argument population tests to use new PasswordUtil API.
app/src/test/java/com/amaze/filemanager/ui/activities/MainActivityTest.kt Update SMB encrypt/decrypt usages for new SmbUtil API.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/test/TestUtils.kt Adjust SSH/FTP test URI building; introduce password URL-encoding in one path.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/test/MockSshConnectionPools.kt Update mocked connection pool storage to use LruCache.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/NetCopyClientConnectionPoolSshTest.kt Update tests for new PasswordUtil signature.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/NetCopyClientConnectionPoolReusabilityTest.kt New test validating connection reuse and LRU cache size behavior.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/FilesOnSshdTest.kt Modernize callback usage (lambda) and remove unused imports.
app/src/test/java/com/amaze/filemanager/filesystem/ssh/AbstractSftpServerTest.kt Update password handling and broaden server authenticator user set; make tearDown open.
app/src/test/java/com/amaze/filemanager/filesystem/ftp/NetCopyClientConnectionPoolTest.kt Add tests for lifecycle shutdown and base-URI canonicalization/reuse.
app/src/test/java/com/amaze/filemanager/filesystem/ftp/NetCopyClientConnectionPoolFtpTest.kt Add connection reuse tests; update password encryption usage and client mock behavior.
app/src/test/java/com/amaze/filemanager/database/UtilsHandlerTest.kt Update SMB path encryption calls for new SmbUtil API.
app/src/test/java/com/amaze/filemanager/database/UtilitiesDatabaseMigrationTest.kt Update encryption/decryption call sites for new PasswordUtil/SmbUtil APIs.
app/src/test/java/com/amaze/filemanager/asynchronous/asynctasks/ssh/SshAuthenticationTaskTest.kt Update pool mocking to LruCache and password encryption calls.
app/src/main/java/com/amaze/filemanager/utils/smb/SmbUtil.kt Remove Context dependency and simplify SMB encrypt/decrypt helpers; use toUri().
app/src/main/java/com/amaze/filemanager/utils/PasswordUtil.kt Remove Context parameter from encrypt/decrypt API and internals.
app/src/main/java/com/amaze/filemanager/utils/PackageUtils.kt Convert to Kotlin object with @JvmStatic for Java callers.
app/src/main/java/com/amaze/filemanager/ui/views/drawer/Drawer.java Update Java calls for PackageUtils API change.
app/src/main/java/com/amaze/filemanager/ui/fragments/preferencefragments/SecurityPrefsFragment.kt Update master password encrypt/decrypt calls for new PasswordUtil API.
app/src/main/java/com/amaze/filemanager/ui/fragments/FtpServerFragment.kt Update FTP server password encrypt/decrypt calls for new PasswordUtil API.
app/src/main/java/com/amaze/filemanager/ui/dialogs/share/ShareTask.java Update Java call for PackageUtils API change.
app/src/main/java/com/amaze/filemanager/ui/dialogs/SmbConnectDialog.java Update SMB dialog encryption/decryption calls for new APIs.
app/src/main/java/com/amaze/filemanager/ui/dialogs/SftpConnectDialog.kt Remove weak context usage; update password encryption/decryption calls; minor UI call cleanups.
app/src/main/java/com/amaze/filemanager/ui/dialogs/AlertDialog.kt Update theme accessor usage.
app/src/main/java/com/amaze/filemanager/ui/activities/MainActivity.java Update Java call for PackageUtils API change.
app/src/main/java/com/amaze/filemanager/filesystem/ftp/NetCopyClientUtils.kt Canonicalize base URIs (sorted query params) and adjust URI derivation and SMB encrypt/decrypt helpers.
app/src/main/java/com/amaze/filemanager/filesystem/ftp/NetCopyClientConnectionPool.kt Refactor pool to LruCache, key by base URI, remove latch usage, and add lifecycle observer.
app/src/main/java/com/amaze/filemanager/filesystem/files/EncryptDecryptUtils.kt Update decrypt call for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/database/typeconverters/EncryptedStringTypeConverter.kt Update Room converters for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/database/UtilitiesDatabase.kt Update migration encryption/decryption calls for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/asynchronous/services/ftp/FtpService.kt Update decrypt call for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/asynchronous/asynctasks/ssh/SshAuthenticationTaskCallable.kt Update decrypt call for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/asynchronous/asynctasks/ftp/auth/FtpsAuthenticationTaskCallable.kt Update decrypt call for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/asynchronous/asynctasks/ftp/auth/FtpAuthenticationTaskCallable.kt Update decrypt call for new PasswordUtil signature.
app/src/main/java/com/amaze/filemanager/asynchronous/asynctasks/ftp/auth/FtpAuthenticationTask.kt Simplify error handling list and remove debug logging in onFinish.
app/src/main/java/com/amaze/filemanager/asynchronous/asynctasks/ftp/AbstractGetHostInfoTask.kt Use requireNotNull for mainActivityContext when showing progress dialog.
app/src/main/java/com/amaze/filemanager/application/AppConfig.java Force early initialization of NetCopyClientConnectionPool.
app/src/androidTest/java/com/amaze/filemanager/filesystem/files/CryptUtilTest.java Update instrumented test for new PasswordUtil signature.
app/src/androidTest/java/com/amaze/filemanager/asynchronous/services/ftp/FtpServiceEspressoTest.kt Update password encryption call for new PasswordUtil signature.
app/build.gradle Add Lifecycle runtime/process dependencies.
.github/workflows/publish-test-results.yaml New workflow to publish unit test results from CI artifacts.
app/src/main/java/com/amaze/filemanager/utils/ImmutableEntry.java Remove ImmutableEntry class (deleted).
Comments suppressed due to low confidence (1)

app/src/main/java/com/amaze/filemanager/filesystem/ftp/NetCopyClientUtils.kt:254

  • deriveUriFrom(...) no longer URL-encodes the password when building the URI. If the (encrypted or plain) password contains reserved characters (notably @, :, /, ?, &), NetCopyConnectionInfo parsing can break because credentials are parsed by delimiter. Consider restoring encoding for the password component (and decode where needed) so generated URIs remain parseable and round-trippable.
        // FIXME: should be caller's responsibility
        var pathSuffix = defaultPath
        if (pathSuffix == null) pathSuffix = SLASH.toString()
        if (explicitTls) pathSuffix = "$pathSuffix?$ARG_TLS=$TLS_EXPLICIT"
        val thisPassword =
            if (password.isNullOrEmpty()) {
                ""
            } else {
                ":$password"
            }
        return if (username == "") {
            "$prefix$hostname:$port$pathSuffix"
        } else {
            "$prefix$username$thisPassword@$hostname:$port$pathSuffix"
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/publish-test-results.yaml Outdated
Comment on lines 401 to +424
val task =
FtpAuthenticationTask(
protocol,
host,
port,
certInfo,
username,
password,
explicitTls,
)
val latch = CountDownLatch(1)
var result: FTPClient? = null
Single.fromCallable(task.getTask())
.subscribeOn(Schedulers.io())
.subscribe({
result = it
latch.countDown()
}, {
latch.countDown()
task.onError(it)
})
latch.await()
return result?.let { ftpClient ->
FTPClientImpl(ftpClient)
}

return kotlin.runCatching {
Single.create { emitter ->
try {
val retval = task.getTask().call()
emitter.onSuccess(retval)
} catch (e: Exception) {
emitter.onError(e)
}
}.map { ftpClient -> FTPClientImpl(ftpClient) }
.subscribeOn(Schedulers.io())
// .observeOn(AndroidSchedulers.mainThread())
.blockingGet()
}.getOrNull()

Copilot AI Mar 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as SSH: createFtpClient(...) converts errors into null without invoking FtpAuthenticationTask.onError(...), so connection failures may become silent regressions. Consider preserving the previous behavior by calling task.onError(e) (on main thread) and/or returning a failure to the caller instead of quietly returning null.

Copilot uses AI. Check for mistakes.
Comment thread gradle.properties Outdated
Comment on lines 345 to +367
val task =
SshAuthenticationTask(
hostname = host,
port = port,
hostKey = hostKey,
username = username,
password = password,
privateKey = keyPair,
)
val latch = CountDownLatch(1)
var retval: SSHClient? = null
Maybe.fromCallable(task.getTask())
.subscribeOn(Schedulers.io())
.subscribe({
retval = it
latch.countDown()
}, {
latch.countDown()
task.onError(it)
})
latch.await()
return retval?.let {
SSHClientImpl(it)
}

return runCatching {
Single.create { emitter ->
try {
val retval = task.getTask().call()
emitter.onSuccess(retval)
} catch (e: Exception) {
emitter.onError(e)
}
}.map { sshClient -> SSHClientImpl(sshClient) }
.subscribeOn(Schedulers.io())
// .observeOn(AndroidSchedulers.mainThread())
.blockingGet()
}.getOrNull()

Copilot AI Mar 5, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

createSshClientInternal now swallows all exceptions (runCatching { ... }.getOrNull()) and no longer calls SshAuthenticationTask.onError(...). This removes user-visible error handling (including HOST_KEY_NOT_VERIFIABLE dialogs/toasts) and makes failures silent. Ensure errors still propagate to onError (on the main thread) or are otherwise surfaced/logged appropriately.

Copilot uses AI. Check for mistakes.
Comment thread app/src/main/java/com/amaze/filemanager/application/AppConfig.java Outdated
@TranceLove TranceLove force-pushed the feature/net-copy-client-connection-pool-improvements branch 2 times, most recently from 22fa469 to 7ffcdac Compare March 8, 2026 15:01
@VishnuSanal VishnuSanal added the PR-Awaiting-Initial-Review this PR is awaiting for an initial review label Jun 28, 2026
@VishnuSanal

Copy link
Copy Markdown
Member

conflicts..

- Use LruCache
- Remove use of CountdownLatch
- Made NetCopyClientConnectionPool lifecycle-aware
- PasswordUtil remove Context parameter
- Use ActivityRule for better lifecycle in Robolectric tests
- Updated AndroidX test dependencies
- TargetApi -> RequiresApi in MainActivity
@TranceLove TranceLove force-pushed the feature/net-copy-client-connection-pool-improvements branch from 7ffcdac to 3e52894 Compare July 4, 2026 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR-Awaiting-Initial-Review this PR is awaiting for an initial review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants