-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix(connectivity_plus): improve network callback unregistration handling #3681
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
base: main
Are you sure you want to change the base?
fix(connectivity_plus): improve network callback unregistration handling #3681
Conversation
@vbuberen , could you please review the PR? The fix will prevent the Android app from crashing. ![]() Thank you! |
Why don't you use the As I see the main change is only
while the rest looks like some AI generated code as well as the PR description itself. Were you able to confirm that this actually solves the bug that your users experience? Since there is no easy way to reproduce the issue I would like to ask you to use the package from your fork and get back later with data on whenever the issue is gone. Also, since you already have the crash in Crashlytics, I would appreciate tech data part from the issue with details on which devices/Android versions it happens. |
Hi @vbuberen , We use GitHub Copilot Enterprise for pull request descriptions in our organisation. We will use our internal fork and see if the issue is resolved. We will come back to you with information. @KirillBorodin, please provide the device and other information requested by @vbuberen. Thanks. |
It is great and many devs use such tools these days, including me, but it doesn't mean that the generated result should just be blindly copy-pasted without any review. |
@vbuberen , could you please kindly point on the "copy-pasted without any review" code in the PR?
This will guarantee that the networkCallback instance is set to Also, the changes improve code readability by splitting the unregistering of the broadcast receiver and network callback, depending on the Android OS version. Thank in advance! |
You don't need to tag person in every single comment, since there will be a notification anyway.
But let's also talk about the code itself:
With that being said I am interested only in confirmation of the fix and not in further discussion on how somebody uses AI tools. It was already obvious from the initial version of the PR. Thus, I would ask to continue the discussion about the topic of the issue only here. |
In addition, a few thoughts: Since the crash indicates that the network callback was not registered, this means that for some reason, We can check when |
PR Description
fix: safely unregister Android network callback to prevent crash when callback already unregistered or null
Problem
The app crashes with
IllegalArgumentException: NetworkCallback was not registered
when the Flutter engine is destroyed, particularly during background work with WorkManager. This happens because Android'sConnectivityManager
checks ifnetworkCallback.networkRequest
is null before unregistering:Since
networkRequest
is an internal field that we cannot check directly, and it can become null in various scenarios (double cleanup, process recreation, etc.), we need to handle the unregistration defensively.Related Issues
#1025
Checklist
- [x] I read the [Contributor Guide](https://github.com/fluttercommunity/plus_plugins/blob/main/CONTRIBUTING.md) and followed the process outlined there for submitting PRs.
- [x] I titled the PR using Conventional Commits.
- [x] I did not modify the
CHANGELOG.md
nor the plugin version inpubspec.yaml
files.- [x] All existing and new tests are passing.
- [x] The analyzer (
flutter analyze
) does not report any problems on my PR.Breaking Change
Does this PR require plugin users to manually update their apps?
- [ ] Yes, this is a breaking change (add
!
in the title).- [x] No, this is *not* a breaking change.