From ff3222ce51d88538cc563b84fb90ae197a89efb0 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 3 Apr 2025 13:34:36 +0900 Subject: [PATCH] Temporarily raise the number of system requests to 375 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a stopgap to palliate a bad interaction between ConnectivityService and JobScheduler. To help developers find request leaks and protect against OOMs in the system server, ConnectivityService is enforncing a maximum number of network requests/callbacks per UID. It has a higher value for system UIDs because some system components need to file requests on behalf of apps, most prominently JobScheduler. The count is increased on entry in ConnectivityService, on the Binder thread ; this is when CS can throw TooManyRequestsException to the caller, it could not do that later when the request is actually registered on the handler thread. Conversely it only decreases the count on the handler thread, because until then it doesn't know whether the request was actually registered, and it can't look at that data structure on the Binder thread. This leads to a slightly confusing result where a UID can register a request immediately after it unregisters one, and it will receive TooManyRequestsException, because the unregister hasn't been processed yet and the count not decremented yet. This is arguably a bug, but if your app is treading on that last request, you almost certainly had a leak anyway, and you should not be trying to play with the exact count of requests either. The remaining case is when an app would unregister and register requests again in a tight loop, but there is no valid use case for that, right ? Right !? Enter JobScheduler. Because the limit for system UIDs is 250 requests but there can be more than 250 UIDs (especially on devices with multiple users), JobScheduler has its own limit of requests (which is set to 125). It has heuristics to choose which are the most important 125 UIDs to file requests for. When the request count exceeds 125, or when something happens that may change which are the most important 125 UIDs, JobScheduler will re-evaluate which are the most important ones. Upon doing this, it will unregister all requests and file again the important ones in a tight loop. At worst, it will therefore remove 125 requests and add 125 requests again, taking up at most all 250 requests allowed for system UIDs ; if the system server has any other request outstanding (and it does), it may hit the issue where CS doesn't know yet that requests have been unregistered, and crashes and hilarity ensue. To palliate the issue above, this patch increases the allowed request count for system UIDs by 125. This immediately should fix the issue where all 250 requests are taken up by JobScheduler. For a long-term fix, we should instead make at least one of the following fixes : - Have JobScheduler be smarter, and make sure it only unregisters the requests that it no longer needs instead of all of them. - Make CS only throw when the requests are really full, by using some combination of locking, making the caller wait or similar. - Have a separate pool of requests for JobScheduler that is dimensioned so it knows it can file all the necessary requests and no longer has to limit itself, and simplify the JS code. Flag: EXEMPT bugfix Bug: 391114053 Test: ConnectivityServiceTest Change-Id: I4c4702e2d0c5d03eb5f3d6fe0f1578a210b188e8 --- service/src/com/android/server/ConnectivityService.java | 7 ++++++- .../java/com/android/server/ConnectivityServiceTest.java | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index c870edc94f..4021a82eff 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -504,8 +504,13 @@ public class ConnectivityService extends IConnectivityManager.Stub { static final int MAX_NETWORK_REQUESTS_PER_UID = 100; // The maximum number of network request allowed for system UIDs before an exception is thrown. + // The point of this limit is to help developers find request leaks, so it should be + // appropriately low, while allowing plenty of requests for normal use cases. + // In the case of the system UIDs, the main consumer of such requests is JobScheduler, which + // at most might file one request per UID on the system. Also see b/391114053 for some context + // about the specific JobScheduler case and its own self-imposed limit. @VisibleForTesting - static final int MAX_NETWORK_REQUESTS_PER_SYSTEM_UID = 250; + static final int MAX_NETWORK_REQUESTS_PER_SYSTEM_UID = 375; @VisibleForTesting protected int mLingerDelayMs; // Can't be final, or test subclass constructors can't change it. diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 5337bfb444..63d014aad9 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -7611,7 +7611,7 @@ public class ConnectivityServiceTest { final int CALLBACKS = 88; final int DIFF_INTENTS = 10; final int SAME_INTENTS = 10; - final int SYSTEM_ONLY_MAX_REQUESTS = 250; + final int SYSTEM_ONLY_MAX_REQUESTS = 375; // CALLBACKS + DIFF_INTENTS + 1 (same intent) // = MAX_REQUESTS - 1, since the capacity is MAX_REQUEST - 1. assertEquals(MAX_REQUESTS - 1, CALLBACKS + DIFF_INTENTS + 1);