android: replace broadcast intent with service intent #650
Conversation
Pull Request Revisions
✅ AI review completed for r2 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at [email protected]. |
![review-ai-agent[bot]](https://avatars.githubusercontent.com/in/1104228?s=60&v=4){kind=small}
| try { | ||
| startService(intent) | ||
| } catch (illegalStateException: IllegalStateException) { | ||
| TSLog.e(TAG, "restartVPN hit IllegalStateException in startService(): $illegalStateException") | ||
| } catch (e: Exception) { | ||
| TSLog.e(TAG, "restartVPN hit exception in startService(): $e") | ||
| } |
There was a problem hiding this comment.
In android/src/main/java/com/tailscale/ipn/App.kt's restartVPN method, consider adding a more specific catch for SecurityException which can occur when the VPN service start is disallowed by the user. This would help distinguish between security-related failures and other runtime exceptions, allowing for more targeted error handling.
| ACTION_RESTART_VPN -> { | ||
| app.setWantRunning(false){ | ||
| close() | ||
| app.startVPN() | ||
| } | ||
| START_NOT_STICKY | ||
| } |
There was a problem hiding this comment.
In android/src/main/java/com/tailscale/ipn/IPNService.kt, the ACTION_RESTART_VPN handling first calls app.setWantRunning(false) followed by close() which no longer contains app.setWantRunning(false). Should we be concerned about this state management change or potential duplicate calls?
| viewModelScope.launch { | ||
| delay(500) // Wait to batch multiple rapid updates | ||
| App.get().updateUserDisallowedPackageNames(excludedPackageNames.value) | ||
| } |
There was a problem hiding this comment.
In android/src/main/java/com/tailscale/ipn/ui/viewModel/SplitTunnelAppPickerViewModel.kt, the pendingChanges set is added but only used for tracking add/remove operations. Since the actual updates are based on excludedPackageNames.value, can you clarify the purpose of this set? It appears unused in the final update operation.
We were previously calling startService(intent), which is a direct call consumed by IPNService, but restartVPN was not working as intended because the broadcast receiver was never triggered. Rather than use a broadcast receiver, directly start the service in restartVPN as we do in stopVPN. Also, batch changes to excluded apps so that we don't restart the VPN each time the user toggles an app. Fixes tailscale/corp#28668 Signed-off-by: kari-ts <[email protected]>
| override fun close() { | ||
| app.setWantRunning(false) {} | ||
| Notifier.setState(Ipn.State.Stopping) | ||
| disconnectVPN() | ||
| Libtailscale.serviceDisconnect(this) |
There was a problem hiding this comment.
The IPNService.kt close() method no longer calls app.setWantRunning(false), while the ACTION_RESTART_VPN handler does this explicitly. Consider abstracting this state management logic to prevent potential inconsistencies in future modifications.
We were previously calling startService(intent), which is a direct call consumed by IPNService, but restartVPN was not working as intended because the broadcast receiver was never triggered. Rather than use a broadcast receiver, directly start the service in restartVPN as we do in stopVPN. Also, batch changes to excluded apps so that we don't restart the VPN each time the user toggles an app.
Fixes tailscale/corp#28668