forked from starifly/NekoBoxForAndroid
-
Notifications
You must be signed in to change notification settings - Fork 0
Keep the network listener active after callback fallback #141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
app/src/main/java/io/nekohasekai/sagernet/utils/NetworkCallbackRegistration.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package io.nekohasekai.sagernet.utils | ||
|
|
||
| internal class NetworkCallbackRegistration { | ||
| internal var isRegistered = false | ||
| private set | ||
| internal var requiresFallback = false | ||
| private set | ||
|
|
||
| fun register(block: () -> Unit): Result<Unit> { | ||
| if (isRegistered) return Result.success(Unit) | ||
| return runCatching(block).onSuccess { | ||
| isRegistered = true | ||
| requiresFallback = false | ||
| } | ||
| } | ||
|
|
||
| fun unregister(block: () -> Unit): Result<Unit> { | ||
| if (!isRegistered) return Result.success(Unit) | ||
| return runCatching(block) | ||
| .onSuccess { | ||
| isRegistered = false | ||
| requiresFallback = false | ||
| } | ||
| .onFailure { throwable -> | ||
| if (throwable is IllegalArgumentException) { | ||
| isRegistered = false | ||
| requiresFallback = false | ||
| } else { | ||
| requiresFallback = true | ||
| } | ||
| } | ||
| } | ||
| } |
107 changes: 107 additions & 0 deletions
107
app/src/test/java/io/nekohasekai/sagernet/utils/NetworkCallbackRegistrationTest.kt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| package io.nekohasekai.sagernet.utils | ||
|
|
||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertFalse | ||
| import org.junit.Assert.assertSame | ||
| import org.junit.Assert.assertTrue | ||
| import org.junit.Test | ||
|
|
||
| class NetworkCallbackRegistrationTest { | ||
|
|
||
| @Test | ||
| fun successfulRegisterThenUnregisterInvokesEachLambdaOnce() { | ||
| val registration = NetworkCallbackRegistration() | ||
| var registerCalls = 0 | ||
| var unregisterCalls = 0 | ||
|
|
||
| val registerResult = registration.register { registerCalls++ } | ||
| val unregisterResult = registration.unregister { unregisterCalls++ } | ||
|
|
||
| assertTrue(registerResult.isSuccess) | ||
| assertTrue(unregisterResult.isSuccess) | ||
| assertEquals(1, registerCalls) | ||
| assertEquals(1, unregisterCalls) | ||
| assertFalse(registration.isRegistered) | ||
| } | ||
|
|
||
| @Test | ||
| fun failedRegisterLeavesStateFalseAndSkipsUnregisterLambda() { | ||
| val registration = NetworkCallbackRegistration() | ||
| val failure = IllegalStateException("registration failed") | ||
| var unregisterCalls = 0 | ||
|
|
||
| val registerResult = registration.register { throw failure } | ||
| val unregisterResult = registration.unregister { unregisterCalls++ } | ||
|
|
||
| assertSame(failure, registerResult.exceptionOrNull()) | ||
| assertTrue(unregisterResult.isSuccess) | ||
| assertEquals(0, unregisterCalls) | ||
| assertFalse(registration.isRegistered) | ||
| } | ||
|
|
||
| @Test | ||
| fun failedUnregisterKeepsStateForRetry() { | ||
| val registration = NetworkCallbackRegistration() | ||
| val failure = IllegalStateException("unregistration failed") | ||
| var successfulUnregisterCalls = 0 | ||
| registration.register {} | ||
|
|
||
| val failedResult = registration.unregister { throw failure } | ||
| val retryResult = registration.unregister { successfulUnregisterCalls++ } | ||
|
|
||
| assertSame(failure, failedResult.exceptionOrNull()) | ||
| assertTrue(retryResult.isSuccess) | ||
| assertEquals(1, successfulUnregisterCalls) | ||
| assertFalse(registration.isRegistered) | ||
| assertFalse(registration.requiresFallback) | ||
| } | ||
|
|
||
| @Test | ||
| fun alreadyUnregisteredFailureAllowsFreshRegistration() { | ||
| val registration = NetworkCallbackRegistration() | ||
| var registerCalls = 0 | ||
| registration.register {} | ||
|
|
||
| val unregisterResult = registration.unregister { | ||
| throw IllegalArgumentException("callback was not registered") | ||
| } | ||
| val registerResult = registration.register { registerCalls++ } | ||
|
|
||
| assertTrue(unregisterResult.isFailure) | ||
| assertTrue(registerResult.isSuccess) | ||
| assertEquals(1, registerCalls) | ||
| assertTrue(registration.isRegistered) | ||
| assertFalse(registration.requiresFallback) | ||
| } | ||
|
|
||
| @Test | ||
| fun registerAfterEitherFailureDoesNotDuplicateARegisteredCallback() { | ||
| val registration = NetworkCallbackRegistration() | ||
| var successfulRegisterCalls = 0 | ||
|
|
||
| registration.register { throw IllegalStateException("registration failed") } | ||
| val afterRegisterFailure = registration.register { successfulRegisterCalls++ } | ||
| registration.unregister { throw IllegalStateException("unregistration failed") } | ||
| val afterUnregisterFailure = registration.register { successfulRegisterCalls++ } | ||
|
|
||
| assertTrue(afterRegisterFailure.isSuccess) | ||
| assertTrue(afterUnregisterFailure.isSuccess) | ||
| assertEquals(1, successfulRegisterCalls) | ||
| assertTrue(registration.isRegistered) | ||
| assertTrue(registration.requiresFallback) | ||
| } | ||
|
|
||
| @Test | ||
| fun repeatedUnregisterWhileFalseIsHarmless() { | ||
| val registration = NetworkCallbackRegistration() | ||
| var unregisterCalls = 0 | ||
|
|
||
| val firstResult = registration.unregister { unregisterCalls++ } | ||
| val secondResult = registration.unregister { unregisterCalls++ } | ||
|
|
||
| assertTrue(firstResult.isSuccess) | ||
| assertTrue(secondResult.isSuccess) | ||
| assertEquals(0, unregisterCalls) | ||
| assertFalse(registration.isRegistered) | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Startonly receives the currentactiveNetwork, so later Wi-Fi/mobile switches or network loss are not delivered throughonAvailableoronLost. After a service restart in this state,underlyingNetworkcan stay stale for the rest of the process unless the old callback is still active.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change. After repeated platform cleanup failures, callback ownership is unknown. Registering again could create duplicate callbacks and conflicting stale events. The listener retries cleanup on each restart, delivers the current active network while uncertain, and keeps the actor alive; conservative fallback is the safe residual behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point — when unregister fails, the old callback's ownership is genuinely ambiguous, and a blind re-register in that state could produce duplicates or conflicting
onAvailable/onLostevents that are harder to reason about than a known-stale snapshot. The retry-on-restart + current-network delivery is a coherent conservative contract.Withdrawing the concern. The fallback behavior is intentional and the reasoning is sound.